-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Update build to use a local package cache from a submodule and build offline #53975
Conversation
@@ -15,9 +15,14 @@ | |||
<PackageVersion>$(VersionPrefix)$(VersionSuffix)</PackageVersion> | |||
</PropertyGroup> | |||
|
|||
<Target Name="Restore"> | |||
<Target Name="_VerifyNPMCache"> |
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.
@javiercn is this the right time to call npm cache verify
(before Restore)?
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.
Yep
Wanted to get your thoughts on if this warrants validating in the VMR prior to merging? I would like to avoid surprises, regressing SB CI, and/or blocking the sdk->installer dependency flow. |
I think that's a good idea. How can I help get that done? |
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.
We also need to remove the checked in JS files, don't we?
Good point, doing that now |
@wtgodbe I'm merging this as I'm doing npm dependency updates as part of build ops and I don't want to create conflicts for you. |
I see this is already merged but for future reference, you can either create a git patch with your changes and apply it to the vmr or use this vmr-sync script. In this particular case involving the submodule, the vmr-sync would be the recommended approach. Once the changes are applied, I would recommend just opening a draft PR and let the CI run to validate. If you would like to build locally, the build instructions are in the VMR's readme |
It would still be good to validate this IMO to ensure this does not block the sdk->installer dependency flow or regress source-build. |
On another look, this won't affect source-build because BuildNodeJS is not set to true in SB. Is enabling this in source-build on someone's radar/tracked in an issue? |
This seems to be causing a failure: dotnet/installer#18641 (comment) |
Responded here: dotnet/installer#18641 (comment) |
It's already enabled in this repo's source-build leg. I'm enabling it in Installer in dotnet/installer#18641 |
This appears to have regressed all of the offline source-build legs in CI - dotnet/source-build#4129 |
@MichaelSimons The error seems to be caused by a component that is likely not needed and that we can avoid using. Is there a way to setup an environment variable in source build? We probably just need to set |
Replaces https://github.com/dotnet/aspnetcore/pull/53524/files to fix a casing issue with the submodule. Should fix #53710.