Skip to content

Always install node & build node components in CI #53154

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

Merged
merged 8 commits into from
Jan 9, 2024
Merged

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jan 4, 2024

Needed as part of the work to move to NPM

@wtgodbe wtgodbe requested a review from javiercn January 4, 2024 20:58
@wtgodbe wtgodbe requested a review from a team as a code owner January 4, 2024 20:58
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 4, 2024
@javiercn
Copy link
Member

javiercn commented Jan 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@@ -536,20 +526,18 @@ stages:
jobName: Linux_musl_x64_build
jobDisplayName: "Build: Linux Musl x64"
agentOs: Linux
container: mcr.microsoft.com/dotnet-buildtools/prereqs:alpine-3.14-WithNode
Copy link
Member

Choose a reason for hiding this comment

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

This thing is pinning the nodejs version and it's using a very old version.

I've switched to 3.17 and let the Install Nodejs task do the right thing.

Is there a reason why we don't have an image that it is based on alpine:latest (3.19) so that we can alternatively install nodejs in the image? (In case we need it). For example, latest comes with nodejs 20.x, which is ideally what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

We just haven't set one up yet. The ownership of creating docker images has always been somewhat obtuse to me, but we're allowed to add images to https://github.com/dotnet/dotnet-buildtools-prereqs-docker if we want. I'd probably just add a new node image to the https://github.com/dotnet/dotnet-buildtools-prereqs-docker/tree/main/src/alpine/3.18 set. What errors were you seeing with the other images you tried out in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

See https://learn.microsoft.com/en-us/azure/devops/pipelines/process/container-phases?view=azure-devops#linux-based-containers

See https://learn.microsoft.com/en-us/azure/devops/pipelines/process/container-phases?view=azure-devops#bring-your-own-nodejs

We should start with something like https://learn.microsoft.com/en-us/azure/devops/pipelines/process/container-phases?view=azure-devops#full-example-of-a-dockerfile but obviously using a modern alpine image 3.19 contains node and can be installed via apk update && apk add nodejs npm which brings in node 20.10.

Ideally what we want is make sure that the node we use in the build is the one that we install, as that "frees us" from having to install it through a separate mechanism.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 5, 2024

@javiercn looks like linux-musl is working & building node components now

@wtgodbe wtgodbe changed the title Always install node in CI Always install node & build node components in CI Jan 5, 2024
@wtgodbe wtgodbe requested a review from a team as a code owner January 8, 2024 07:56
@javiercn javiercn force-pushed the wtgodbe/InstallNode branch from 471f9f9 to 75fa7ad Compare January 8, 2024 07:59
@javiercn
Copy link
Member

javiercn commented Jan 8, 2024

@wtgodbe looks great so far!

I've pushed a small extra "experimental" commit to delete the checked-in files and see if it breaks anywhere and there's something that we missed.

If that mostly works (the only thing I expect to break at this point is source build), then, we can do either 2 things:

  • Revert my commit and get this merged.
  • Fix source build in this PR (maybe we could add an image like the one you have in the tag here) and merge all of it.
    • We can then have a small follow up issue to update the image tags to stable versions.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Try removing build condition for node projects

@javiercn javiercn force-pushed the wtgodbe/InstallNode branch from 39f827d to dc11a2a Compare January 8, 2024 09:13
* [Blazor] Remove checked-in JS files
* Update .gitignore and fix paths
* Update images to Centos8
* Update ubuntu images
* Make sure NPM scripts build inside sourcebuild
* Disable running Firefox on Mac OS
@wtgodbe wtgodbe requested review from BrennanConroy, halter73 and a team as code owners January 8, 2024 20:28
@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 9, 2024

#53228

@wtgodbe wtgodbe merged commit 0bc8a2f into main Jan 9, 2024
@wtgodbe wtgodbe deleted the wtgodbe/InstallNode branch January 9, 2024 01:49
@ghost ghost added this to the 9.0-preview1 milestone Jan 9, 2024
@@ -83,4 +83,35 @@
</ItemGroup>
</Target>

<Target Name="RestoreNpmPackages"
Copy link
Member

Choose a reason for hiding this comment

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

What are the prereqs for this? I am wondering if changes are going to be needed to the containers used in SB CI. This should be flushed out to prevent blocking the dependency flow into installer.

Copy link
Member

Choose a reason for hiding this comment

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

The image needs node 20 (the LTS) version to be part of the image.

Copy link
Member

Choose a reason for hiding this comment

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

Only one of the container images used in SB CI has node - ubuntu 22.04 and it is extremely old. The list is specified here - https://github.com/dotnet/installer/blob/main/eng/pipelines/templates/stages/vmr-build.yml#L19 Can someone from aspnet add node prior to this flowing into installer?

Regarding the version - has there been any discussion with @dotnet/distro-maintainers to ensure this is the minimum version supported across the distros that will source-build .NET 9.0? @leecow this is something to raise in the next source-build partner sync.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @MichaelSimons ! Since this becomes a build-time dependency with minimum version, I agree on calling this out with distros and partners. After all, this impacts whether/where we can build .NET 9.

Hopefully, it shouldn't be an issue this time around.

https://github.com/nodejs/release#release-schedule is a good resource for Node.js versioning. The last version before 20 (18) goes EOL early 2025 - so just a few months after .NET 9 GA. I don't think using that is really an option for .NET 9, unless a rebase is on the table during a servicing release just a few months post GA. Even Node.js 20 goes EOL in mid-2026, (almost) aligning with the expected .NET 9 EOL.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MichaelSimons we've already added Node to centos-stream8, as well as new alpine image - alpine-3.19-WithNode. Does every image in that list need to have node added?

Copy link
Member

Choose a reason for hiding this comment

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

We need to test on each of these distros. We can update to a newer version per what .NET 9 will support when it releases (e.g. updating to the new Alpine 3.19 image)

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha - so to make sure I understand:

  • alpine-3.17: can be updated to alpine-3.19-WithNode
  • centos-stream8: already has Node 20
  • centos-stream9, fedora-39, ubuntu-22.04. ubuntu-22.04-arm64 : need to add Node 20

Does that sound right? If so I can do that today.

Copy link
Member

Choose a reason for hiding this comment

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

Does that sound right? If so I can do that today.

Yes that sounds correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants