aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Don't check in blazor.*.js files

Open wtgodbe opened this issue 2 years ago • 4 comments

Should resolve https://github.com/dotnet/aspnetcore/issues/11592

CC @mkArtakMSFT @crummel

wtgodbe avatar Jan 04 '23 22:01 wtgodbe

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

wtgodbe avatar Jan 04 '23 22:01 wtgodbe

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?

https://github.com/dotnet/aspnetcore/blob/005f935632967cc28db269950afe667512d5012b/.azure/pipelines/ci.yml#L366

wtgodbe avatar Jan 04 '23 22:01 wtgodbe

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

BrennanConroy avatar Jan 05 '23 17:01 BrennanConroy

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

javiercn avatar Jan 05 '23 17:01 javiercn

@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?

wtgodbe avatar Jan 06 '23 00:01 wtgodbe

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

javiercn avatar Jan 09 '23 11:01 javiercn

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!

crummel avatar Jan 09 '23 20:01 crummel

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?

wtgodbe avatar Jan 11 '23 23:01 wtgodbe

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

javiercn avatar Jan 12 '23 09:01 javiercn

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.

crummel avatar Jan 12 '23 16:01 crummel

@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?

wtgodbe avatar Jan 12 '23 21:01 wtgodbe

@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 avatar Jan 12 '23 22:01 MattGal

@wtgodbe some notes:

  • Both "good" and "bad" ASP.NET core runs are using sha 83f779a1faf182ba7ceec453fd02869bfba13abb37f17e354ac4fd91338bb4a6 of the mcr.microsoft.com/dotnet-buildtools/prereqs:alpine-3.14-WithNode image, so this doesn't seem likely to be your root cause.
  • https://github.com/microsoft/azure-pipelines-tasks/tree/master/Tasks/NodeToolV0 hasn't been updated any time recently
  • Ubuntu 20.04 hosted image updated on 20230109.1, but shouldn't matter for jobs run inside containers

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!

MattGal avatar Jan 12 '23 22:01 MattGal

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?

mirespace avatar Jan 16 '23 12:01 mirespace

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 avatar Jan 16 '23 19:01 omajid

@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 avatar Jan 16 '23 19:01 omajid

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?

omajid avatar Jan 16 '23 21:01 omajid

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.

dougbu avatar Jan 17 '23 01:01 dougbu

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?

omajid avatar Jan 17 '23 23:01 omajid

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.

dougbu avatar Jan 17 '23 23:01 dougbu

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

omajid avatar Jan 17 '23 23:01 omajid

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.

dougbu avatar Jan 18 '23 00:01 dougbu

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 avatar Jan 25 '23 15:01 ghost

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.

omajid avatar Jan 25 '23 15:01 omajid

Closing as this is blocked on moving to NPM

wtgodbe avatar Feb 14 '23 17:02 wtgodbe