All changes together without offline #20504

cstamas · 2024-01-30T10:08:42Z

All changes from #20495 but removed "offline".

Changes:

GH actions: remove "offline" stuff, no need to "prime local repository" either (go-offline and related removed)
removed takari-local-repository, instead using RRF + file locking + BF collector
mvnw changed to "script-only"

wendigo · 2024-01-30T10:18:58Z

@cstamas I wanted to remove go offline for some time as it was causing spurious failures in unexpected scenarios. Thanks for working on this! We want to stay on the latest Maven to be able to move to 4.0.0 when it's out.

cstamas · 2024-01-30T10:48:17Z

Problem on master is:

presence of takari-local-repository, if "just removed" fun things happen (and breakage)
RRF makes some of those issues fixed, other bits fixed are Maven 3.9.x file locking
fun starts when we enable smart builder across all, but we need some plugin changes, see Update plugin trino-maven-plugin#23
maybe proviso plugin needs some as well, still looking....

cstamas · 2024-01-30T11:43:31Z

All fine in CI

In the meantime Provisio got PR as well jvanzyl/provisio#65

cstamas · 2024-01-30T11:50:05Z

~~Now comes the fun part: enable smart builder for all... (trino-maven-plugin and provisio will yell, but IMHO all should be fine... if not, i'll just take a step back).~~ Took a step back, seems tests dislike MT build, so let leave it ST for now...

wendigo · 2024-01-30T19:22:56Z

@cstamas I'll update trino-maven-plugin here: #20514

cstamas · 2024-01-30T19:46:24Z

@wendigo hope you can release proviso as well (I left a comment to be done before merge), and then with both updated will sync to master.

cstamas · 2024-02-01T11:31:48Z

Updated PR, @wendigo pls ping if provisio new release done and merged, Thanks! (or just done, we can updated it as part of this PR even)

wendigo · 2024-02-01T11:38:16Z

@cstamas I've asked @jvanzyl over the Slack to do a release.

cstamas · 2024-02-04T15:19:02Z

Removed central prefixes, basically we are back to "allow all for central" (as I see we do not care for 'leaking reqs' toward Central, which is OK).

cstamas · 2024-02-13T15:27:27Z

Updated PR, now latest trino-maven-plugin and proviso used.

wendigo · 2024-02-14T18:54:39Z

@nineinchnick ptal

nineinchnick · 2024-02-14T19:56:01Z

.mvn/maven.config

-
+-Daether.syncContext.named.factory=file-lock
+-Daether.syncContext.named.nameMapper=file-hgav
+-Daether.syncContext.named.retry=5


I can't find any reference docs for these properties, does this mean downloads will be retried?

https://maven.apache.org/resolver/configuration.html

Thanks, can you add it in a comment in this file? I see aether.connector.http.retryHandler.count has a default value of 3.

Were these retries always available in Maven?

Since Maven 3.9.x, with introduction of locking

In Resolver 2 this page is generated and contains more data, like "since" and "source": https://maven.apache.org/resolver-archives/resolver-2.0.0-alpha-7/configuration.html

BUT, do not use this page are reference for Resolver 1.x (Maven 3.x) as there has been renamed property keys! Still, if you can match the property (unchanged since 1.x or figure out what was renamed from) you can figure out "since".

nineinchnick · 2024-02-14T19:56:17Z

.mvn/maven.config

+-Daether.syncContext.named.factory=file-lock
+-Daether.syncContext.named.nameMapper=file-hgav
+-Daether.syncContext.named.retry=5
+-Daether.syncContext.named.time=30


What's the unit of this?

aether.syncContext.named.time.unit by default is TimeUnit.SECONDS

aether.syncContext.named.time already has a default of 30, remove it or add the unit too to make it easier to read.

nineinchnick

This is great, I'm very happy to remove the download-maven-dependencies.sh script. This unblocks some other work. Thanks again!

cstamas · 2024-02-15T08:54:25Z

Last plugin that needs some love:

[WARNING] *****************************************************************
[WARNING] The following plugins are not marked as thread-safe in trino-server-rpm:
[WARNING]   io.airlift.maven.plugins:redlinerpm-maven-plugin:2.1.8
[WARNING] 
[WARNING] Enable debug to see precisely which goals are not marked as thread-safe.
[WARNING] *****************************************************************

cstamas · 2024-02-15T08:56:49Z

Pushed two more commits:

"slight" introduction of smart builder (was unused, just added to extensions.xml: now where -T is used, -b smart is used as well)
apply PR comments: placed doco URL and removed def value

wendigo

LGTM % some unrelated changes to undo

wendigo · 2024-02-15T13:42:16Z

.github/actions/setup/action.yml

@@ -3,16 +3,13 @@ description: "Verify checked out commits and setup Java"
 inputs:
  java-version:
    description: "Java version to setup"
-    default: 21


revert, irrelevant change

wendigo · 2024-02-15T13:42:21Z

.github/actions/setup/action.yml

  cache:
    description: "Cache Maven repo (true/false/restore)"
-    default: true


revert, irrelevant change

.github/actions/setup/action.yml

wendigo · 2024-02-15T13:42:58Z

.github/workflows/ci.yml

    runs-on: ubuntu-latest
-    needs: build-test-matrix


revert, irrelevant change

wendigo · 2024-02-15T13:43:03Z

.github/workflows/ci.yml

@@ -937,6 +940,8 @@ jobs:
          echo "matrix=$(cat matrix.json)" >> $GITHUB_OUTPUT

  pt:
+    needs:


revert, irrelevant change

wendigo · 2024-02-15T13:44:05Z

mvnw

@@ -21,288 +21,219 @@
 # ----------------------------------------------------------------------------
 # Apache Maven Wrapper startup batch script, version 3.2.0
 #
-# Required ENV vars:


Why this has changed? It's unrelated, please undo

Maven Wrapper has several "flavors", and I personally prefer "only-script", see here https://maven.apache.org/wrapper/#usage-without-binary-jar

Naturally, all of those flavors have different mvnw/mvnw.cmd scripts. The "only-script" simply delegates to distro mvn/mvn.cmd script, and hence, supports full feature set that selected distro supports. The currently used wrapper is the "bin" one (has also a JAR file checked in, this PR removes that) that has several shortcomings (like https://issues.apache.org/jira/browse/MWRAPPER-110 etc).

Hence, in short: this PR changes Maven Wrapper type from "bin" to "only-script" and that has an additional "cost" of actual changes in mvn/mvnw.cmd scripts (as unlike in bin, they just delegate).

Can you extract this to a separate commit?

We tend to split changes into smaller, atomic commits (and we test that each commit separately compiles) and keep commit messages in sync with the actual commit content.

wendigo · 2024-02-15T13:45:58Z

@cstamas please squash commits 1 and 3 together. 2 can be a separate one

cstamas · 2024-02-19T20:06:37Z

Created #20504 for mvnw

Changes: * GH actions: remove "offline" stuff, no need to "prime local repository" either (go-offline and related removed) * removed takari-local-repository, instead using RRF + file locking + BF collector * mvnw changed to "script-only"

wendigo · 2024-02-19T21:10:09Z

@cstamas mvnw merged

wendigo · 2024-02-20T05:39:26Z

I've amended the commit message (to reword and drop mvn wrapper mention as it was extracted to a separate commit) and pushed directly to master as f133a42. Thank you @cstamas !

nineinchnick · 2024-02-20T09:38:08Z

.github/workflows/ci.yml

@@ -97,7 +97,8 @@ jobs:
                format('refs/pull/{0}/head', github.event.client_payload.pull_request.number) || '' }}
      - uses: ./.github/actions/setup
        with:
-          cleanup-node: true
+          cache: 'restore'


Looks like no job populates the cache anymore, all uses of ./.github/actions/setup are done with cache: restore.

findepi · 2024-03-11T20:22:25Z

.github/workflows/ci.yml

@@ -18,14 +18,13 @@ env:
  # An envar that signals to tests we are executing in the CI environment
  CONTINUOUS_INTEGRATION: true
  # allow overriding Maven command
-  MAVEN: ./mvnw --offline
-  # maven.wagon.rto is in millis, defaults to 30m
-  MAVEN_OPTS: "-Xmx512M -XX:+ExitOnOutOfMemoryError -Dmaven.wagon.rto=60000"


Why -Xmx512M -XX:+ExitOnOutOfMemoryError was removed?
is this related to removal of the go-offline?

findepi · 2024-03-11T20:22:35Z

.github/workflows/ci.yml

  MAVEN_GIB: "-P gib -Dgib.referenceBranch=refs/remotes/origin/${{ github.event_name == 'pull_request' && github.event.pull_request.base.ref || github.event.repository.default_branch }}"
-  MAVEN_TEST: "-B --strict-checksums -Dmaven.source.skip=true -Dair.check.skip-all --fail-at-end -P gib -Dgib.referenceBranch=refs/remotes/origin/${{ github.event_name == 'pull_request' && github.event.pull_request.base.ref || github.event.repository.default_branch }}"


is --strict-checksums no longer necessary?

cla-bot bot added the cla-signed label Jan 30, 2024

cstamas force-pushed the all-changes-together-without-offline branch from 8518a91 to 0373da0 Compare January 30, 2024 15:15

cstamas force-pushed the all-changes-together-without-offline branch from 0373da0 to ce3ff03 Compare February 1, 2024 10:26

cstamas force-pushed the all-changes-together-without-offline branch from ce3ff03 to 5bda71a Compare February 4, 2024 15:18

cstamas force-pushed the all-changes-together-without-offline branch from 5bda71a to e4ac503 Compare February 13, 2024 15:25

wendigo requested review from nineinchnick and electrum February 13, 2024 15:28

cstamas force-pushed the all-changes-together-without-offline branch from e4ac503 to 9e04442 Compare February 14, 2024 17:08

cstamas marked this pull request as ready for review February 14, 2024 17:08

nineinchnick reviewed Feb 14, 2024

View reviewed changes

nineinchnick approved these changes Feb 15, 2024

View reviewed changes

This was referenced Feb 15, 2024

Make the build lean #20495

Closed

Use maven-resolver locking instead of takari #20487

Closed

Make project buildable with Maven4 #20478

Closed

Enable RRF #20477

Closed

wendigo approved these changes Feb 15, 2024

View reviewed changes

cstamas force-pushed the all-changes-together-without-offline branch 2 times, most recently from 0559cc9 to 51416d1 Compare February 19, 2024 20:04

cstamas force-pushed the all-changes-together-without-offline branch from 51416d1 to 5ba934f Compare February 19, 2024 20:21

cstamas force-pushed the all-changes-together-without-offline branch from 5ba934f to 56a94ee Compare February 19, 2024 20:23

wendigo closed this Feb 20, 2024

nineinchnick reviewed Feb 20, 2024

View reviewed changes

nineinchnick mentioned this pull request Feb 20, 2024

Populate the cache in the Maven Checks job #20760

Merged

findepi mentioned this pull request Feb 20, 2024

Fix builds with maven daemon #20762

Merged

cstamas deleted the all-changes-together-without-offline branch February 20, 2024 12:07

findepi reviewed Mar 11, 2024

View reviewed changes

		MAVEN_GIB: "-P gib -Dgib.referenceBranch=refs/remotes/origin/${{ github.event_name == 'pull_request' && github.event.pull_request.base.ref \|\| github.event.repository.default_branch }}"
		MAVEN_TEST: "-B --strict-checksums -Dmaven.source.skip=true -Dair.check.skip-all --fail-at-end -P gib -Dgib.referenceBranch=refs/remotes/origin/${{ github.event_name == 'pull_request' && github.event.pull_request.base.ref \|\| github.event.repository.default_branch }}"

All changes together without offline #20504

All changes together without offline #20504

Conversation

cstamas commented Jan 30, 2024 • edited Loading

wendigo commented Jan 30, 2024

cstamas commented Jan 30, 2024 • edited Loading

cstamas commented Jan 30, 2024

cstamas commented Jan 30, 2024 • edited Loading

wendigo commented Jan 30, 2024

cstamas commented Jan 30, 2024 • edited Loading

cstamas commented Feb 1, 2024

wendigo commented Feb 1, 2024

cstamas commented Feb 4, 2024

cstamas commented Feb 13, 2024

wendigo commented Feb 14, 2024

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

cstamas Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstamas Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nineinchnick left a comment

Choose a reason for hiding this comment

cstamas commented Feb 15, 2024

cstamas commented Feb 15, 2024 • edited Loading

wendigo left a 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

wendigo Feb 15, 2024 • edited Loading

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

wendigo commented Feb 15, 2024

cstamas commented Feb 19, 2024

wendigo commented Feb 19, 2024

wendigo commented Feb 20, 2024

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

cstamas commented Jan 30, 2024 •

edited

Loading

cstamas commented Jan 30, 2024 •

edited

Loading

cstamas commented Jan 30, 2024 •

edited

Loading

cstamas commented Jan 30, 2024 •

edited

Loading

cstamas Feb 15, 2024 •

edited

Loading

cstamas Feb 14, 2024 •

edited

Loading

cstamas commented Feb 15, 2024 •

edited

Loading

wendigo Feb 15, 2024 •

edited

Loading