Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Oct 2, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1366ddd
Use eirslett/frontend-maven-plugin#1157
basil Sep 22, 2024
cf1fb1e
Test 1
basil Sep 19, 2024
167eee1
Test 2
basil Sep 19, 2024
7d07e78
Test 3
basil Sep 19, 2024
a98a3de
Testing is finished
basil Sep 19, 2024
2f79deb
Merge branch 'master' into corepack
basil Sep 23, 2024
246170c
https://github.com/jenkinsci/jenkins/pull/9772#discussion_r1771275590
basil Sep 23, 2024
3b7922c
Merge branch 'master' into corepack
basil Sep 23, 2024
c6a69c8
https://github.com/jenkinsci/jenkins/pull/9772#discussion_r1771977848
basil Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,7 @@ junit.xml

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

Choose a reason for hiding this comment

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

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

.yarn/*
.yarnrc.yml
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

!.yarn/patches
!.yarn/plugins
!.yarn/sdks
Expand All @@ -78,7 +76,4 @@ node/
node_modules/

# Generated JavaScript Bundles
jsbundles
Copy link
Member Author

Choose a reason for hiding this comment

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

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


# In case someone accidentally runs npm install instead of yarn install
package-lock.json
Comment on lines -83 to -84
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

war/src/main/webapp/jsbundles/
2 changes: 0 additions & 2 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ node/

.git

.yarnrc.yml
Copy link
Member Author

Choose a reason for hiding this comment

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

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


# libraries / external deps / generated files
war/src/main/js/plugin-setup-wizard/bootstrap-detached.js
war/src/main/webapp/scripts/yui
Expand Down
2 changes: 2 additions & 0 deletions .yarnrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
enableGlobalCache: false
Copy link
Member

Choose a reason for hiding this comment

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

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

(not a blocker)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

nodeLinker: node-modules
Comment on lines +1 to +2
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

4 changes: 2 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ MAVEN_OPTS='--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/ja
### Running the Yarn frontend build

> [!TIP]
> If you already have Node.js installed, you do not need to change your path. Start using `yarn` by enabling [Corepack](https://yarnpkg.com/corepack) with `corepack enable`, if it isn't already; this will add the `yarn` binary to your PATH.
> If you already have Node.js 20.x LTS installed, you do not need to change your path. Start using Yarn by enabling [Corepack](https://yarnpkg.com/corepack) with `corepack enable`, if it isn't already; this will add the `yarn` binary to your path.
timja marked this conversation as resolved.
Show resolved Hide resolved

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
Copy link
Member Author

Choose a reason for hiding this comment

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

Adapting to the changes in pom.xml below.

```

Then you can run Yarn with e.g.
Expand Down
55 changes: 12 additions & 43 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,6 @@ THE SOFTWARE.
<!-- Make sure to keep the jetty-ee9-maven-plugin version in war/pom.xml in sync with the Jetty release in Winstone: -->
<winstone.version>8.1</winstone.version>
<node.version>20.17.0</node.version>
<!-- frontend-maven-plugin will install this Yarn version as bootstrap, then hand over control to Yarn Berry. -->
<yarn.version>1.22.19</yarn.version>
Comment on lines -102 to -103
Copy link
Member Author

Choose a reason for hiding this comment

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

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

<!-- maven-antrun-plugin will download this Yarn version. -->
<yarn-berry.version>4.5.0</yarn-berry.version>
<yarn-berry.sha256sum>cc00dce5de4f68d11450519a0f69eadf2a1cbe5cc0d8e740bfac817a31d76874</yarn-berry.sha256sum>
Comment on lines -104 to -106
Copy link
Member Author

Choose a reason for hiding this comment

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

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

</properties>

<!--
Expand Down Expand Up @@ -430,66 +425,40 @@ THE SOFTWARE.
</activation>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-antrun-plugin</artifactId>
<executions>
<execution>
<id>download-yarn</id>
<goals>
<goal>run</goal>
</goals>
<phase>initialize</phase>
<configuration>
<target>
<property name="yarn.dest" value="${project.basedir}/.yarn/releases/yarn-${yarn-berry.version}.cjs" />
<dirname file="${yarn.dest}" property="yarn.dest.dir" />
<mkdir dir="${yarn.dest.dir}" />
<get dest="${yarn.dest}" src="https://repo.yarnpkg.com/${yarn-berry.version}/packages/yarnpkg-cli/bin/yarn.js" usetimestamp="true" />
<checksum algorithm="SHA-256" file="${yarn.dest}" property="${yarn-berry.sha256sum}" verifyProperty="yarn.checksum.matches" />
<condition property="yarn.checksum.matches.fail">
<equals arg1="${yarn.checksum.matches}" arg2="false" />
</condition>
<fail if="yarn.checksum.matches.fail">Checksum error</fail>
<echo file="${project.basedir}/.yarnrc.yml">yarnPath: ${yarn.dest}</echo>
</target>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>com.github.eirslett</groupId>
<artifactId>frontend-maven-plugin</artifactId>
<version>1.15.1</version>
<executions>
<execution>
<id>install node and yarn</id>
<id>install node and corepack</id>
<goals>
<goal>install-node-and-yarn</goal>
<goal>install-node-and-corepack</goal>
</goals>
<phase>initialize</phase>
<configuration>
<nodeVersion>v${node.version}</nodeVersion>
<yarnVersion>v${yarn.version}</yarnVersion>
<nodeDownloadRoot>https://repo.jenkins-ci.org/nodejs-dist/</nodeDownloadRoot>
<!-- tried to create a mirror for yarnDownloadRoot but it did not work -->
</configuration>
</execution>
<execution>
<id>yarn install</id>
<goals>
<goal>yarn</goal>
<goal>corepack</goal>
</goals>
<phase>initialize</phase>
<configuration>
<arguments>yarn install</arguments>
</configuration>
</execution>
<execution>
<id>yarn build</id>
<goals>
<goal>yarn</goal>
<goal>corepack</goal>
</goals>
<phase>generate-sources</phase>
<configuration>
<arguments>build</arguments>
<arguments>yarn build</arguments>
</configuration>
</execution>
</executions>
Expand Down Expand Up @@ -517,11 +486,11 @@ THE SOFTWARE.
<execution>
<id>yarn lint:ci</id>
<goals>
<goal>yarn</goal>
<goal>corepack</goal>
</goals>
<phase>test</phase>
<configuration>
<arguments>lint:ci</arguments>
<arguments>yarn lint:ci</arguments>
<skip>${yarn.lint.skip}</skip>
</configuration>
</execution>
Expand Down Expand Up @@ -550,11 +519,11 @@ THE SOFTWARE.
<execution>
<id>yarn lint</id>
<goals>
<goal>yarn</goal>
<goal>corepack</goal>
</goals>
<phase>test</phase>
<configuration>
<arguments>lint</arguments>
<arguments>yarn lint</arguments>
<skip>${yarn.lint.skip}</skip>
</configuration>
</execution>
Expand Down
1 change: 1 addition & 0 deletions test/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ THE SOFTWARE.
<jenkins.addOpens>--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED</jenkins.addOpens>
<!-- Filled in by maven-hpi-plugin with the path to org-netbeans-insane-hook.jar extracted from jenkins-test-harness -->
<jenkins.insaneHook />
<test>hudson.AboutJenkinsTest</test>
</properties>

<dependencyManagement>
Expand Down
20 changes: 0 additions & 20 deletions war/.gitignore
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion war/src/main/scss/components/_side-panel-tasks.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
margin: var(--section-padding);
gap: 0.125rem;

/* stylelint-disable-next-line media-query-no-invalid */
@media (min-width: breakpoints.$tablet-breakpoint) {

Check warning on line 13 in war/src/main/scss/components/_side-panel-tasks.scss

View check run for this annotation

ci.jenkins.io / Stylelint

media-query-no-invalid 

ERROR:
Unexpected invalid media query "(min-width: breakpoints.$tablet-breakpoint)" (media-query-no-invalid)
margin-right: calc($background-outset);
}
}
Expand Down
Loading