Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add debian support #2119

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

add debian support #2119

wants to merge 23 commits into from

Conversation

curllog
Copy link

@curllog curllog commented Feb 5, 2025

related to #2120

@curllog curllog changed the title add distro support add debian support Feb 5, 2025
@curllog curllog marked this pull request as draft February 5, 2025 21:03
@curllog curllog marked this pull request as ready for review February 5, 2025 21:56
@curllog
Copy link
Author

curllog commented Feb 5, 2025

@curllog please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@nagilson nagilson self-assigned this Feb 10, 2025
@nagilson
Copy link
Member

This is very exciting 🚀 Thank you for making this, it's awesome to see community interest and engagement here. I will add taking a look at this PR to my list.

@curllog
Copy link
Author

curllog commented Feb 10, 2025

This is very exciting 🚀 Thank you for making this, it's awesome to see community interest and engagement here. I will add taking a look at this PR to my list.

thanks, this feature will improve developer experiene on debian distro, i hope it can get more priority

@nagilson
Copy link
Member

Absolutely, I was OOF since I was moving but this will be prioritized.

@nagilson nagilson mentioned this pull request Feb 11, 2025
Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contributing, I'm certainly excited to see demand for this feature on other distros. It takes a good amount of time to read through our docs to understand how it works, so I appreciate your efforts here. I have some thoughts before I think we can merge this, please see them below. Thanks for being patient!

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const versionData = distroVersions.filter((x: { [x: string]: string; }) => x[this.versionKey] === String(targetVersion));
return versionData;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the json file was changed, I was able to call the API once to install the sdk which is awesome ❇️ It did produce some extraneous output though which I don't see in other versions.
This is due to the PMC feed not being needed by other distros or being handled differently.

For Debian,
I wonder if the package microsoft feed injection needs to happen before dotnetPackageExistsOnSystem runs.
image

Newer versions of Ubuntu don't require the PMC feed added by the function injectPMCFeed(). I think you may want to try calling this function during the packages check.

You could try to override the packages check command and then call the parent function after trying this. There may also be a better approach.

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const versionData = distroVersions.filter((x: { [x: string]: string; }) => x[this.versionKey] === String(targetVersion));
return versionData;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I noticed, is that if we try to install 8 and then 9 on debian without restarting and very soon after each other, the 2nd attempt may fail because the dpkg lock is held. I think this is a broader issue for the entire codebase so it's not a blocker for your PR, but it may be worth looking into. One of those apt-get running processes doesn't seem to be dying: dpkg: error: dpkg frontend lock was locked by another process with pid blah

You can check the hanging process which is in the output with something like ps -P pid, which might be valuable information to get.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may actually have been triggered by the package check failure above.

Copy link
Member

@nagilson nagilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you may credit yourself in the changelog.md if you wish. If any of your work is based on: https://github.com/dotnet/vscode-dotnet-runtime/pull/2050/files this may also be worth noting.

@curllog
Copy link
Author

curllog commented Feb 12, 2025

Also, you may credit yourself in the changelog.md if you wish. If any of your work is based on: https://github.com/dotnet/vscode-dotnet-runtime/pull/2050/files this may also be worth noting.

you mentioned community debian support, should i add my username? i don't know best practice about crediting myself :)

@nagilson
Copy link
Member

Also, you may credit yourself in the changelog.md if you wish. If any of your work is based on: https://github.com/dotnet/vscode-dotnet-runtime/pull/2050/files this may also be worth noting.

you mentioned community debian support, should i add my username? i don't know best practice about crediting myself :)

I can take care of that part, no prob, as long as you want to be credited 👍

@curllog
Copy link
Author

curllog commented Feb 12, 2025

Also, you may credit yourself in the changelog.md if you wish. If any of your work is based on: https://github.com/dotnet/vscode-dotnet-runtime/pull/2050/files this may also be worth noting.

you mentioned community debian support, should i add my username? i don't know best practice about crediting myself :)

I can take care of that part, no prob, as long as you want to be credited 👍

Also, you may credit yourself in the changelog.md if you wish. If any of your work is based on: https://github.com/dotnet/vscode-dotnet-runtime/pull/2050/files this may also be worth noting.

you mentioned community debian support, should i add my username? i don't know best practice about crediting myself :)

I can take care of that part, no prob, as long as you want to be credited 👍

i appreciate it, thanks

@nagilson
Copy link
Member

It looks like some of the tests are failing because debian is not supported even on the Mac Pipeline which is interesting. The linter also needs to be resolved.

@curllog
Copy link
Author

curllog commented Feb 13, 2025

It looks like some of the tests are failing because debian is not supported even on the Mac Pipeline which is interesting. The linter also needs to be resolved.

should i take an action? if yes can i get more information?

@nagilson
Copy link
Member

Yes, please try to get the PR to a green status on the pipeline. You can see the failure details for each part here:
image
image

You can also run npm lint yourself to see if it works (you might need to run npm install.) One issue is a linter problem with await being called, the other is a test failure I think its caused by this line const pair: DistroVersionPair = { distro: 'Debian', version: '12' }; as the string Debian here is wrong. You may need to investigate it further by running the tests though.

@curllog
Copy link
Author

curllog commented Feb 15, 2025

@nagilson Hi again, all tests are passed, can this pr be merged or it needs more modification?

@curllog
Copy link
Author

curllog commented Feb 18, 2025

@nagilson all tests are passed, unfortunately after my last commit linux test had timeout error on pipeline, i coudn't rerun the pipeline manually

@nagilson
Copy link
Member

Srry about that, let me rerun it

@curllog
Copy link
Author

curllog commented Feb 18, 2025

@nagilson now all tests are passed, should i change something or we are good to go?

@nagilson
Copy link
Member

@nmschulte-aviture agarwalishita If either of you would also like to be credited for this in an update, please let me know.

@nagilson
Copy link
Member

It doesn't seem to work on a new debian VM because it doesnt call inject pmc feed before myVersionPackages? Maybe. Thats a bug in my code though so im working to fix it, I'll be working on this!

@curllog
Copy link
Author

curllog commented Feb 22, 2025

@nagilson Hi do we have any update on this? should i take any action?

@curllog
Copy link
Author

curllog commented Feb 24, 2025

@nagilson unfortunately we got timeout for last pipeline again

@nagilson
Copy link
Member

nagilson commented Feb 27, 2025

Still working on this but it still has some sort of bug.

Feed injection is now at least completing but searching the cache for dotnet-sdk-8.0 packages etc is still returning empty so it thinks no versions are supported. I will try to look into this in a bit but you may try as well. If you try in a clean debian machine, it still does not work yet.

Separately, I think dotnetRecommendedVersionRegistration is throwing before it finishes the version parsing after this fix.. this is an unrelated bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants