Skip to content

[Build] Run Maven build in parallel and only up until the verify phase #1010

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

Merged
merged 1 commit into from
Jan 29, 2024
Merged

[Build] Run Maven build in parallel and only up until the verify phase #1010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion .github/workflows/maven.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ jobs:
with:
run: >-
mvn --batch-mode -V -U
--threads 1C
-DforkCount=1
-Dnative=${{ matrix.config.native }}
-Dcompare-version-with-baselines.skip=true
Expand All @@ -71,7 +72,7 @@ jobs:
--fail-at-end
-DskipNativeTests=false
-DfailIfNoTests=false
clean install
clean verify
- name: Performance tests
if: contains(github.event.pull_request.labels.*.name, 'performance')
uses: coactions/setup-xvfb@b6b4fcfb9f5a895edadc3bc76318fae0ac17c8b3 # v1.0.1
Expand Down
2 changes: 1 addition & 1 deletion Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ pipeline {
dir('eclipse.platform.swt') {
sh '''
mvn clean verify \
--batch-mode -DforkCount=0 \
--batch-mode --threads 1C -DforkCount=0 \
-Dcompare-version-with-baselines.skip=false -Dmaven.compiler.failOnWarning=true \
-Dorg.eclipse.swt.tests.junit.disable.test_isLocal=true \
-Dmaven.test.failure.ignore=true -Dmaven.test.error.ignore=true
Expand Down
3 changes: 2 additions & 1 deletion tests/org.eclipse.swt.tests.cocoa/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ Bundle-Version: 3.108.100.qualifier
Bundle-Vendor: Eclipse.org
Bundle-Localization: plugin
Require-Bundle: org.junit;bundle-version="4.12.0",
org.eclipse.swt
org.eclipse.swt,
org.eclipse.swt.tests
Eclipse-BundleShape: dir
Bundle-RequiredExecutionEnvironment: JavaSE-17
Automatic-Module-Name: org.eclipse.swt.tests.cocoa
3 changes: 2 additions & 1 deletion tests/org.eclipse.swt.tests.gtk/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ Bundle-SymbolicName: org.eclipse.swt.tests.gtk
Bundle-Version: 3.109.0.qualifier
Bundle-Vendor: Eclipse.org
Require-Bundle: org.junit;bundle-version="4.12.0",
org.eclipse.swt
org.eclipse.swt,
org.eclipse.swt.tests
Eclipse-BundleShape: dir
Bundle-RequiredExecutionEnvironment: JavaSE-17
Automatic-Module-Name: org.eclipse.swt.tests.gtk
3 changes: 2 additions & 1 deletion tests/org.eclipse.swt.tests.win32/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ Bundle-Version: 3.108.100.qualifier
Bundle-Vendor: Eclipse.org
Bundle-Localization: plugin
Require-Bundle: org.junit;bundle-version="4.12.0",
org.eclipse.swt
org.eclipse.swt,
org.eclipse.swt.tests
Copy link
Contributor

@HeikoKlare HeikoKlare Jan 30, 2024

Choose a reason for hiding this comment

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

The problem with such a dependency is that it only makes sense in the context of a parallel Maven build. It does something else than what these dependency specifications are supposed to be used for, so no one except the ones involved in this PR will know that (and probably even they will forget), so there will be people trying to understand the necessity for this dependency and eventually even remove it.

I would at least suggest to add a comment in the Manifest that explains the necessity for the dependency.

Copy link
Contributor

@laeubi laeubi Jan 30, 2024

Choose a reason for hiding this comment

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

I would at least suggest to add a comment in the Manifest that explains the necessity for the dependency.

Manifest can't have comments. Another alternative would be additional.bundles in the build.properties or a dependency in the pom.xml or a p2.inf

Copy link
Contributor

@HeikoKlare HeikoKlare Jan 30, 2024

Choose a reason for hiding this comment

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

Manifest can't have comments

Just out of interest: did that change at some point in time? Because in a former project, several years ago, I had comments in a Manifest (but I do not remember the syntax).

Copy link
Member Author

@HannesWell HannesWell Jan 30, 2024

Choose a reason for hiding this comment

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

Manifest can't have comments

Just out of interest: did that change at some point in time? Because in a former project, several years ago, I had comments in a Manifest (but I do not remember the syntax).

It didn't change. But an OSGi framework is supposed to ignore Headers it doesn't understand. So you can have a Comment: Some remark header as long as all characters are permitted.

In general I agree with your concern, that's why I added an explanation in the commit message (that is then hopefully found).
But moving it to the build.properties is probably the best solution here.
Will prepare a follow-up tonight.

Copy link
Contributor

@laeubi laeubi Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A header is not a comment... manifests are meant to be processed by tools not humans that why the specification is rather "odd" and its wide use makes its near to impossible to change (e.g. the stupid no more than 72 bytes (not characters) rule) even though I don't know any manifest parser that enforces the rule (including java)

Copy link
Contributor

@HeikoKlare HeikoKlare Jan 30, 2024

Choose a reason for hiding this comment

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

Thanks for clarification. Now I remember that we used a Comment: ... header, knowing that it was actually a misuse.

Copy link
Member Author

@HannesWell HannesWell Jan 30, 2024

Choose a reason for hiding this comment

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

A header is not a comment... manifests are meant to be processed by tools not humans

That's right. But just because one can doesn't mean one should do something. :)

Copy link
Member Author

@HannesWell HannesWell Jan 30, 2024

Choose a reason for hiding this comment

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

Created #1017.

Eclipse-BundleShape: dir
Bundle-RequiredExecutionEnvironment: JavaSE-17
Automatic-Module-Name: org.eclipse.swt.tests.win32