-
Notifications
You must be signed in to change notification settings - Fork 312
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
base: main
Are you sure you want to change the base?
add debian support #2119
Conversation
@dotnet-policy-service agree |
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 |
Absolutely, I was OOF since I was moving but this will be prioritized. |
There was a problem hiding this 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; | ||
} |
There was a problem hiding this comment.
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.
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; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Co-authored-by: Noah Gilson <OTAKUPENGUINOP@GMAIL.COM>
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 |
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. |
vscode-dotnet-runtime-library/src/Acquisition/LinuxVersionResolver.ts
Outdated
Show resolved
Hide resolved
should i take an action? if yes can i get more information? |
@nagilson Hi again, all tests are passed, can this pr be merged or it needs more modification? |
@nagilson all tests are passed, unfortunately after my last commit linux test had timeout error on pipeline, i coudn't rerun the pipeline manually |
Srry about that, let me rerun it |
@nagilson now all tests are passed, should i change something or we are good to go? |
@nmschulte-aviture agarwalishita If either of you would also like to be credited for this in an update, please let me know. |
It doesn't seem to work on a new debian VM because it doesnt call inject pmc feed before |
@nagilson Hi do we have any update on this? should i take any action? |
@nagilson unfortunately we got timeout for last pipeline again |
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 |
related to #2120