Download Yarn Berry with Corepack rather than Yarn 1.x #9772

basil · 2024-09-22T22:43:10Z

Decrease the maintenance burden in this repository by replacing our custom Ant (!) based code to bootstrap Yarn Berry using Yarn 1.x with Corepack as implemented in eirslett/frontend-maven-plugin#1157.

Testing done

Introduced stylelint and eslint failures in both CI and local builds. In both cases, verified that the error message was readable: in local builds, on the CLI; and in CI builds, in the Jenkins UI.

For the local builds, tested without path modifications on my local machine with Node LTS installed as well as with path modifications inside of a Docker container with no existing Node/NPM installation, thus exercising both supported paths as documented in CONTRIBUTING.md.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

basil · 2024-09-22T22:43:39Z

.gitignore

@@ -65,9 +65,7 @@ junit.xml

 # Yarn
 # https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored
-.pnp.*


See the comments to .yarnrc.yml below for explanation.

basil · 2024-09-22T22:44:24Z

.gitignore

 .yarn/*
-.yarnrc.yml


Previously automatically generated on the fly by our custom Ant-based logic. This Ant-based logic is now deleted, and instead this file is checked into Git. This removes a layer of indirection and makes the code simpler, since the file that is used is now static rather than dynamically generated.

basil · 2024-09-22T22:44:38Z

.gitignore

@@ -78,7 +76,4 @@ node/
 node_modules/

 # Generated JavaScript Bundles
-jsbundles


Using a more specific path to jsbundles, since there is only a single such directory.

basil · 2024-09-22T22:45:21Z

.gitignore

-# In case someone accidentally runs npm install instead of yarn install
-package-lock.json


Using NPM is not supported, and having code in the source tree to handle an unsupported configuration by silently ignoring it (rather than failing fast) violates the principle of least surprise.

basil · 2024-09-22T22:45:37Z

.prettierignore

@@ -7,8 +7,6 @@ node/

 .git

-.yarnrc.yml


No longer automatically generated, so we can adhere to Prettier formatting rules now.

basil · 2024-09-22T22:48:16Z

.yarnrc.yml

+enableGlobalCache: false
+nodeLinker: node-modules


As recommended in https://github.com/eirslett/frontend-maven-plugin/blob/4a501a10677716a0e6676b603e3b93bc6bf319f1/frontend-maven-plugin/src/it/corepack-provided-integration/.yarnrc.yml. Note that without the nodeLinker line eirslett/frontend-maven-plugin#1157 does not actually work, instead complaining about corepack not being listed in package.json. I filed for another day any latent curiosity about why this might be the case (probably a deficiency in frontend-maven-plugin documentation at the very least).

why not using 'pnp' https://yarnpkg.com/migration/pnp

I see no reason why using PNP would solve the problem statement described in this PR description.

CONTRIBUTING.md

basil · 2024-09-22T22:51:23Z

CONTRIBUTING.md


 To run the Yarn frontend build, after [building the WAR file](#building-the-war-file), add the downloaded versions of Node and Yarn to your path:

 ```sh
-export PATH=$PWD/node:$PWD/node/yarn/dist/bin:$PATH
+export PATH=$PWD/node:$PWD/node/node_modules/corepack/shims:$PATH


Adapting to the changes in pom.xml below.

basil · 2024-09-22T22:51:46Z

war/.gitignore

@@ -1,20 +0,0 @@
-work


Unneeded now that the corresponding entries are in the root directory's .gitignore file.

basil · 2024-09-22T23:09:26Z

pom.xml

-    <!-- frontend-maven-plugin will install this Yarn version as bootstrap, then hand over control to Yarn Berry. -->
-    <yarn.version>1.22.19</yarn.version>


Yarn 1.x is now no longer needed to bootstrap Yarn Berry, since we now use Corepack for this purpose.

basil · 2024-09-22T23:09:45Z

pom.xml

-    <!-- maven-antrun-plugin will download this Yarn version. -->
-    <yarn-berry.version>4.5.0</yarn-berry.version>
-    <yarn-berry.sha256sum>cc00dce5de4f68d11450519a0f69eadf2a1cbe5cc0d8e740bfac817a31d76874</yarn-berry.sha256sum>


This duplicate information is no longer needed, as this information is consumed from package.json via Corepack.

scherler · 2024-09-23T16:36:26Z

Tried to build but it is failing

basil · 2024-09-23T17:09:19Z

Tried to build but it is failing

That seems like evidence that you have failed to build Jenkins core, but it does not seem like evidence that there is a general issue with this PR, as the Testing Done section is thorough, including both local testing and CI testing. I am inclined to believe there is something wrong on your side, but that this should not block us from proceeding with this PR.

timja · 2024-09-23T19:33:41Z

My build failed with:

[INFO] Downloading https://registry.npmjs.org/corepack/-/corepack-provided.tgz to /Users/timja/.m2/repository/com/github/eirslett/corepack/provided/corepack-provided.tar.gz
[INFO] No proxies configured
[INFO] No proxy was configured, downloading directly
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for Jenkins main module 2.478-SNAPSHOT:
[INFO]
[INFO] Jenkins main module ................................ FAILURE [  3.534 s]
[INFO] Jenkins BOM ........................................ SKIPPED
[INFO] Internal SPI for WebSocket ......................... SKIPPED
[INFO] Jetty 12 (EE 9) implementation for WebSocket ....... SKIPPED
[INFO] Jenkins cli ........................................ SKIPPED
[INFO] Jenkins core ....................................... SKIPPED
[INFO] Jenkins war ........................................ SKIPPED
[INFO] Tests for Jenkins core ............................. SKIPPED
[INFO] Jenkins coverage ................................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  5.905 s
[INFO] Finished at: 2024-09-23T20:31:22+01:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.github.eirslett:frontend-maven-plugin:1.15.1:install-node-and-corepack (install node and corepack) on project jenkins-parent: Could not download corepack: Got error code 404 from the server. -> [Help 1]
[ERROR]

Steps I followed were:

gh pr checkout 9772
mvn clean install -P quick-build

Env:

jenkins [🌱 corepack][$?][☕ v17][⬢ v20.9.0][🐪 v5.34.0][🐍 v3.12.5][⏱ 7s]
❯ pwd
/Users/timja/code/jenkins/jenkins

Apple M2, on Sequoia

timja · 2024-09-23T19:41:59Z

I managed to fix it by running rm -rf node/

and then the next downloaded version was setup correctly.

I wasn't able to reproduce it by trying to revert to my previous state:

rm -rf node/
git checkout master
mvn clean install -P quick-build
git checkout -
mvn clean install -P quick-build

timja · 2024-09-23T19:44:25Z

.yarnrc.yml

@@ -0,0 +1,2 @@
+enableGlobalCache: false


What's the reasoning for disabling this? it seems to be enabled by default.

(not a blocker)

This was copied from the example here: https://github.com/eirslett/frontend-maven-plugin/blob/4a501a10677716a0e6676b603e3b93bc6bf319f1/frontend-maven-plugin/src/it/corepack-provided-integration/.yarnrc.yml

There is no comment there explaining the reason, but if I had to guess, it would be to follow the general philosophy of frontend-maven-plugin, as explained in its README:

Node/corepack and any package managers will only be "installed" locally to your project. It will not be installed globally on the whole system (and it will not interfere with any Node/corepack installations already present).

The design seems to be to remain completely isolated from the system installation of Node/Corepack, and I suppose the developer was trying to adhere to that philosophy here by avoiding the sharing of any caches with the system installation.

basil · 2024-09-23T19:58:45Z

I managed to fix it

OK, your original error message is definitely related to this PR, and I am glad you were able to solve it by cleaning the working directory. I had tested with a clean working directory as well. Just to be sure, I ran a build successfully in a Docker container without any existing Maven cache, and it worked correctly there, so I think this PR has no issues on a clean environment. So I think we will just need to make sure that developers who have an existing checkout run git clean -fxd or similar before trying to build after this PR. If I am wrong, though, and there is some persistent failure with this PR that is not present before this PR, I would investigate that.

timja

Thanks!

daniel-beck · 2024-09-24T07:44:27Z

It's not a new phenomenon that moving around JS related files breaks the local build with unclean contents. The same happened due to #9718 when switching between "before" and "after" branches, and the same happened a few years ago. Perhaps this just is the first time it happens when moving forward rather than backward.

basil · 2024-09-24T17:43:13Z

It's not a new phenomenon that moving around JS related files breaks the local build with unclean contents. The same happened due to #9718 when switching between "before" and "after" branches, and the same happened a few years ago. Perhaps this just is the first time it happens when moving forward rather than backward.

Indeed, this is yet another ancient build bug. I will try to fix mvn clean to work properly with frontend-maven-plugin in a PR after this one.

timja · 2024-09-25T13:52:13Z

/label ready-for-merge

This PR is now ready for merge, after the pending security release, we will merge it if there's no negative feedback.

Thanks!

Use eirslett/frontend-maven-plugin#1157

1366ddd

basil added the skip-changelog Should not be shown in the changelog label Sep 22, 2024

basil commented Sep 22, 2024

View reviewed changes

basil added 2 commits September 22, 2024 15:52

Test 1

cf1fb1e

Test 2

167eee1

basil commented Sep 22, 2024

View reviewed changes

basil added 2 commits September 22, 2024 16:17

Test 3

7d07e78

Testing is finished

a98a3de

basil marked this pull request as ready for review September 22, 2024 23:31

basil added 2 commits September 23, 2024 07:36

Merge branch 'master' into corepack

2f79deb

https://github.com/jenkinsci/jenkins/pull/9772#discussion_r1771275590

246170c

basil requested a review from scherler September 23, 2024 14:40

basil requested review from timja and removed request for scherler September 23, 2024 17:09

timja changed the title ~~Use eirslett/frontend-maven-plugin#1157~~ Switch downloading yarn to corepack Sep 23, 2024

timja reviewed Sep 23, 2024

View reviewed changes

basil added 2 commits September 23, 2024 12:48

Merge branch 'master' into corepack

3b7922c

https://github.com/jenkinsci/jenkins/pull/9772#discussion_r1771977848

c6a69c8

basil changed the title ~~Switch downloading yarn to corepack~~ Download Yarn Berry with Corepack rather than Yarn 1.x Sep 23, 2024

timja approved these changes Sep 23, 2024

View reviewed changes

timja requested a review from a team September 23, 2024 20:28

scherler approved these changes Sep 25, 2024

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 25, 2024

MarkEWaite merged commit ddad8af into jenkinsci:master Oct 2, 2024
16 checks passed

basil mentioned this pull request Oct 2, 2024

mvn clean should clean frontend artifacts #9815

Merged

basil deleted the corepack branch October 2, 2024 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Download Yarn Berry with Corepack rather than Yarn 1.x #9772

Download Yarn Berry with Corepack rather than Yarn 1.x #9772

basil commented Sep 22, 2024 •

edited

Loading

Submitter checklist

Maintainer checklist

basil Sep 22, 2024

basil Sep 22, 2024

basil Sep 22, 2024

basil Sep 22, 2024

basil Sep 22, 2024

basil Sep 22, 2024

scherler Sep 23, 2024

basil Sep 23, 2024

basil Sep 22, 2024

basil Sep 22, 2024

basil Sep 22, 2024

basil Sep 22, 2024

scherler commented Sep 23, 2024

basil commented Sep 23, 2024

timja commented Sep 23, 2024

timja commented Sep 23, 2024 •

edited

Loading

timja Sep 23, 2024

basil Sep 23, 2024

basil commented Sep 23, 2024

timja left a comment

daniel-beck commented Sep 24, 2024 •

edited

Loading

basil commented Sep 24, 2024

timja commented Sep 25, 2024

		# In case someone accidentally runs npm install instead of yarn install
		package-lock.json

		<!-- frontend-maven-plugin will install this Yarn version as bootstrap, then hand over control to Yarn Berry. -->
		<yarn.version>1.22.19</yarn.version>

		enableGlobalCache: false
		nodeLinker: node-modules

Download Yarn Berry with Corepack rather than Yarn 1.x #9772

Download Yarn Berry with Corepack rather than Yarn 1.x #9772

Conversation

basil commented Sep 22, 2024 • edited Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scherler commented Sep 23, 2024

basil commented Sep 23, 2024

timja commented Sep 23, 2024

timja commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil commented Sep 23, 2024

timja left a comment

Choose a reason for hiding this comment

daniel-beck commented Sep 24, 2024 • edited Loading

basil commented Sep 24, 2024

timja commented Sep 25, 2024

basil commented Sep 22, 2024 •

edited

Loading

timja commented Sep 23, 2024 •

edited

Loading

daniel-beck commented Sep 24, 2024 •

edited

Loading