Skip to content

Don't check in blazor.*.js files #45883

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

Closed
wants to merge 7 commits into from
Closed

Don't check in blazor.*.js files #45883

wants to merge 7 commits into from

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jan 4, 2023

Should resolve #11592

CC @mkArtakMSFT @crummel

@wtgodbe wtgodbe requested a review from a team as a code owner January 4, 2023 22:20
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jan 4, 2023
@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 4, 2023

Weird, this works for me locally - @javiercn any idea why it might not in CI?

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 4, 2023

Oh, I see, it's failing on all legs where we don't build NodeJS. So we'd have to turn the Node build on everywhere if we want to not check these files in. @dougbu @javiercn thoughts?

-noBuildNodeJS

@BrennanConroy
Copy link
Member

Did the source build issues get resolved? I don't see any discussion on it in the linked issues.

@javiercn
Copy link
Member

javiercn commented Jan 5, 2023

@BrennanConroy we discussed offline on an email thread with the source build folks

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 6, 2023

@javiercn I forgot the email thread about this mentioned needing to check in a lockfile with the closure of dependencies all linux distros might need - can you help with that?

@javiercn
Copy link
Member

javiercn commented Jan 9, 2023

@wtgodbe we already check-in the lock files. That's what ensures repeatable builds.

@crummel
Copy link

crummel commented Jan 9, 2023

I'm going to try building this in the source-build full-product context to make sure it doesn't break anything - thanks for pinging me on this!

@wtgodbe wtgodbe requested review from a team and dougbu as code owners January 10, 2023 23:06
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Also lots of failures related to embedded resources

</Target>

<Target Name="_AddEmbeddedBlazor" AfterTargets="_CheckBlazorServerJSPath">
<ItemGroup>
<EmbeddedResource Include="$(BlazorServerJSFile)" LogicalName="_framework/$(BlazorServerJSFilename)" />
<EmbeddedResource Include="$(BlazorServerJSFile)" LogicalName="_framework/$(BlazorServerJSFilename)" Condition="'$(BuildNodeJS)' != 'false' and '$(BuildingInsideVisualStudio)' != 'true'"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this is the file we're deleting, it won't exist unless you build nodeJs. Maybe it'd be better to condition on the file existing, in case a dev follows a NodeJs build with a NoBuildNodeJs build.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 11, 2023

error [email protected]: The engine "node" is incompatible with this module. Expected version ">=12.0.0". Got "10.24.1"

This is only happening on Arm - we installed Node 16.x before this. @javiercn any idea what might be happening?

@javiercn
Copy link
Member

@wtgodbe no idea, can we run node --version to see what the machine prints?

@crummel
Copy link

crummel commented Jan 12, 2023

cc @dotnet/distro-maintainers

@omajid or @mirespace, Would you be able to help out with verifying this change in a source-build context? It'd be nice to get confirmation that the approach works for you and we'd appreciate the help with how tight on resources we are right now.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 12, 2023

@javiercn node --version on Musl gives the following immediately after installing node 16.x:

/__w/_temp/f5c5264c-3db8-4ff5-8493-cab567b71d45.sh: line 1: /__t/node/16.19.0/x64/bin/node: No such file or directory

Even though the preceding installation step claims to have installed to that dir. Something weird going on on the agent? @MattGal any idea what might be happening?

@MattGal
Copy link
Member

MattGal commented Jan 12, 2023

@javiercn node --version on Musl gives the following immediately after installing node 16.x:

/__w/_temp/f5c5264c-3db8-4ff5-8493-cab567b71d45.sh: line 1: /__t/node/16.19.0/x64/bin/node: No such file or directory

Even though the preceding installation step claims to have installed to that dir. Something weird going on on the agent? @MattGal any idea what might be happening?

I do not. I do note that random PRs-to-main do and don't use the install node JS task, but since you're using hosted build agents and the install node task, either could have changed out from under us and caused a problem. I'll spend a few minutes investigating and let you know if I find anything interesting.

@MattGal
Copy link
Member

MattGal commented Jan 12, 2023

@wtgodbe some notes:

Given this, I think there may just be a problem with your build pipelines when installNodeJs is set to true. I'd experiment with listing the contents of the __t directory to see what actually got in there. In linux, sometimes this can actually mean a file ownership or other permission problem.

Good luck!

@mirespace
Copy link

cc @dotnet/distro-maintainers

@omajid or @mirespace, Would you be able to help out with verifying this change in a source-build context? It'd be nice to get confirmation that the approach works for you and we'd appreciate the help with how tight on resources we are right now.

Hi @crummel ! Happy to help. Do you need a complete build, or can the change be tested with a building subset? If the latest, could you provide the args for MSBuild/build.sh or point me to where I can find it?

@omajid
Copy link
Member

omajid commented Jan 16, 2023

I tried testing the VMR with this PR manually applied and was running into issues. Then I realized that one of the transitive dependency packages in package.json, even before this PR, requires access to an internal Microsoft feed:

$ node --version
v18.12.1
$ npm --version
8.19.2
$ cd dotnet/aspnetcore
$ git rev-parse HEAD
6f1752a798a9460b8a039750e30b827578528c90
$ cd src/Components/Web.JS/ 
$ sed -i -E 's|link:|file:|g' package.json
$ npm install
npm ERR! code E401
npm ERR! Unable to authenticate, your authentication token seems to be invalid.
npm ERR! To correct this please trying logging in again with:
npm ERR!     npm login

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/omajid/.npm/_logs/2023-01-16T19_45_06_593Z-debug-0.log
$ tail -25 /home/omajid/.npm/_logs/2023-01-16T19_45_06_593Z-debug-0.log
1988 timing reifyNode:@types/dotnet/node_modules/typescript Completed in 2820ms
1989 timing reify:rollback:createSparse Completed in 634ms
1990 timing reify:rollback:retireShallow Completed in 0ms
1991 timing command:install Completed in 37970ms
1992 verbose stack HttpErrorAuthUnknown: Unable to authenticate, need: Basic realm="https://pkgsprodcus2.pkgs.visualstudio.com/", Bearer, Bearer authorization_uri=https://login.windows.net/npm_***, TFS-Federated
1992 verbose stack     at /usr/lib/node_modules/npm/node_modules/npm-registry-fetch/lib/check-response.js:80:17
1992 verbose stack     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
1993 verbose statusCode 401
1994 verbose pkgid table@https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public-npm/npm/registry/table/-/table-6.8.1.tgz
1995 verbose cwd /home/omajid/devel/dotnet/aspnetcore/src/Components/Web.JS
1996 verbose Linux 6.0.15-300.fc37.x86_64
1997 verbose node v18.12.1
1998 verbose npm  v8.19.2
1999 error code E401
2000 error Unable to authenticate, your authentication token seems to be invalid.
2001 error To correct this please trying logging in again with:
2001 error     npm login
2002 verbose exit 1
2003 timing npm Completed in 38056ms
2004 verbose unfinished npm timer reify 1673898306677
2005 verbose unfinished npm timer reify:unpack 1673898341190
2006 verbose unfinished npm timer reifyNode:node_modules/table 1673898341213
2007 verbose code 1
2008 error A complete log of this run can be found in:
2008 error     /home/omajid/.npm/_logs/2023-01-16T19_45_06_593Z-debug-0.log

@omajid
Copy link
Member

omajid commented Jan 16, 2023

@mirespace, it would probably be better to make sure the VMR with the addition of the changes proposed by this PR works end to end.

@omajid
Copy link
Member

omajid commented Jan 16, 2023

requires access to an internal Microsoft feed

So, I commented out the registry and was able to pull all the nodejs deps needed. I will continue trying to test this end-to-end.

But that has made me think: are there any npm dependencies that are only available via the Microsoft feed? In a future CVE release (Patch Tuesday) is it possible that there are fixes to npm modules that are not part of the aspnetcore repo and only available via pkgs.dev.azure.com?

@dougbu
Copy link
Contributor

dougbu commented Jan 17, 2023

All npm packages must be sourced from the dotnet-public-npm feed, at least in our regular CI. I think that means the VMR as well but defer to @mmitche there.

@omajid authentication requirements when updating lock files is only part of what's going wrong for you. In addition, dependencies in the src/Components/Web.JS/ directory are downloaded using yarn and the yarn.lock file that's checked in there.

I suggest patching the yarn.lock file instead. Then run yarn install --frozen-lockfile. That should avoid yarn thinking it needs to check for new versions.


Side note: @dotnet/aspnet-blazor-eng and @dotnet/aspnet-build please do not merge changes that add other feeds in our lock files. We're hoping to enforce this in the CI's Code Check job soon.

@omajid
Copy link
Member

omajid commented Jan 17, 2023

I just realized that yarn isn't available in some of the environments that we would want to build ASP.NET Core in, such as RHEL 8. npm seems to be available, though. From what I can tell, npm should be able to use the same files?

@dougbu
Copy link
Contributor

dougbu commented Jan 17, 2023

I just realized that yarn isn't available in some of the environments that we would want to build ASP.NET Core in, such as RHEL 8. npm seems to be available, though.

Not available at all or not available w/o a manual installation step in advance?

From what I can tell, npm should be able to use the same files?

Not exactly. Both start w/ the same package.json files but lock files don't share names or formats. I don't think either can read lock files created by the other. (@javiercn or @BrennanConroy is that right❔)

We have thought about switching back to npm but not done the work.

@omajid
Copy link
Member

omajid commented Jan 17, 2023

Not available at all or not available w/o a manual installation step in advance?

Not available in the distro repositories.

Which basically means not available at all. Manual installation is not an option when building in a distro-context for distros that care about building everything from source - like Debian, Fedora and RHEL and Ubuntu. Though RHEL may be the only distro in that list that doesn't include yarn.

Both start w/ the same package.json files but lock files don't share names or formats. I don't think either can read lock files created by the other.

The npm install docs suggest it might be possible:

Description

This command installs a package and any packages that it depends on. If the package has a package-lock, or an npm shrinkwrap file, or a yarn lock file, the installation of dependencies will be driven by that, respecting the following order of precedence:

  • npm-shrinkwrap.json
  • package-lock.json
  • yarn.lock

@dougbu
Copy link
Contributor

dougbu commented Jan 18, 2023

The npm install docs suggest it might be possible:

Might be worth a try in your environment then. That is, see if editing the yarn.lock file and npm ci work for the scenario you were testing. As long as new packages or package versions aren't needed in dotnet-public-npm, all should be well.

@ghost
Copy link

ghost commented Jan 25, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please remove the pending ci rerun label to kick off a new CI run.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 25, 2023
@omajid
Copy link
Member

omajid commented Jan 25, 2023

Might be worth a try in your environment then.

So I did. It looks like I can download the dependencies? The Yarn.MSBuild nuget package includes a bundled copy of yarn (ugh) and it does seem like it starts building things.

I am now running into a strange error that I am trying to figure out:

             yarn version v1.22.10                                                                                                                            
             info Current version: 5.0.0-dev                                                                                                                  
             info Visit https://yarnpkg.com/en/docs/cli/version for documentation about this command.                                                                      error Invalid version supplied.

@wtgodbe
Copy link
Member Author

wtgodbe commented Feb 14, 2023

Closing as this is blocked on moving to NPM

@wtgodbe wtgodbe closed this Feb 14, 2023
@wtgodbe wtgodbe deleted the wtgodbe/NoJS branch February 14, 2023 17:53
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build blazor.*.js in CI; don't commit the artifacts
8 participants