GitHub pull requests should trigger CI

[simon04]

What steps will reproduce the problem?

1. Work on a large feature/patch
2. Open a GitHub pull request

What is the expected result?

CI via GitHub Actions runs

What happens instead?

CI via GitHub Actions does not run

Please provide any additional information below. Attach a screenshot if possible.

Example: ​https://github.com/openstreetmap/josm/pull/70

[Stereo]

So you'd like all the unit tests to run on a github PR? I like that idea.

It currently fails because there will always already have a release based on the svn commit number.

We could have synthetic release numbers, like 12345-simon04:LinkEnvironment but I have no idea how the various bits would react to the build id not being an int.

[simon04]

Why is the release necessary for running unit tests? I'd be happy to have all tests run on Java 8 / Linux / headless.

I'm always very stressed out when a few unit tests fail after doing an SVN commit. I'd like to know this in advance, instead.

The same hods true when reviewing patches: you never know which tests fail (unless you're spending 30min to have the tests run locally, being blocked for everything else).

[Stereo]

The two are indeed separate things, but I thought it'd be nice to have a jar to try out your nice new feature?

[simon04]

I don't care about the generated JAR. Btw, do we need to create a new release for every CI run? (Having 231 temporary releases on ​https://github.com/openstreetmap/josm/releases does not seem to be overly helpful to me in the long run.)

[Stereo]

It's certainly useful to have builds to ask someone to try, and more convenient to have a full .app (or exe/deb/rpm) than just a nightly .jar you have to wait 24 hours for.

We could clean up old pre-releases, if you wanted to. Not many benefits to us.

[simon04]

Actually, GitHub Actions start to run on pull requests, but fail rather quickly since createrelease fails, see ​https://github.com/openstreetmap/josm/pull/70/checks?check_run_id=2144212270

This is a regression of one of the patches applied via #20146, likely r17373.

I'd vote to split .github/workflows/ant.yml into:

• an action that runs ant test (using the java/os/headless matrix), and
• an action that creates the GitHub releases, and builds various jar/app files (only picking one java version, only headless).

[Don-vip]

Replying to simon04:

I'd vote to split .github/workflows/ant.yml into:

• an action that runs ant test (using the java/os/headless matrix), and
• an action that creates the GitHub releases, and builds various jar/app files (only picking one java version, only headless).

Agreed, those are two very different things. Go ahead :)

[simon04]

@Stereo, would you be interested in unscrambling tests and builds/releases via GitHub Actions.

Btw: GitHub Actions supports ​storing workflow data as artifacts (this might be a nice alternative to the temporary releases)

[stoecker]

Before we extend CI on Github could it be fixed first? The multi-daily failure mails are really disturbing.

[simon04]

Yeah, sigh, ... Debugging the CI is supper annoying due to #19889.

I don't want to add more CI actions, but to separate the exiting actions into test and build/deploy.

Maybe we should disable every non-working environment (Java/OS/headless combination) for now, and re-enable them step by step?

Btw: Appveyor is testing every commit twice due to the GitHub pre-release being created for each commit – ​https://ci.appveyor.com/project/don-vip/josm/history

[Stereo]

We actually already use artifacts to upload ant test reports. See the bottom of ​https://github.com/openstreetmap/josm/actions/runs/697729205 for example.

I wouldn't mind working on it, but I would like to please get svn access. It's just really tedious otherwise.

[simon04]

In 17690/josm:

see #20621 - GitHub Actions: formatting for readability

[simon04]

In 17691/josm:

see #20621 - GitHub Actions: remove broken headless=false jobs

The headless=false jobs (via xvfb-run) via GitHub Actions terminate after 6h unsuccessfully.

[simon04]

Any idea why DownloadWmsAlongTrackActionTest.testTMSLayer fails on GitHub Actions? – ​https://github.com/openstreetmap/josm/runs/2226030659

org.openstreetmap.josm.gui.progress.ProgressException: Expected states are [IN_TASK, IN_SUBTASK] but current state is FINISHED
	at org.openstreetmap.josm.gui.progress.AbstractProgressMonitor.checkState(AbstractProgressMonitor.java:84)
	at org.openstreetmap.josm.gui.progress.AbstractProgressMonitor.internalUpdateProgress(AbstractProgressMonitor.java:266)
	at org.openstreetmap.josm.gui.progress.AbstractProgressMonitor.setTicks(AbstractProgressMonitor.java:244)
	at org.openstreetmap.josm.gui.progress.AbstractProgressMonitor.worked(AbstractProgressMonitor.java:258)
	at org.openstreetmap.josm.gui.layer.AbstractTileSourceLayer$PrecacheTask.tileLoadingFinished(AbstractTileSourceLayer.java:1910)
	at org.openstreetmap.josm.data.imagery.TMSCachedTileLoaderJob.loadingFinished(TMSCachedTileLoaderJob.java:219)
	at org.openstreetmap.josm.data.cache.JCSCachedTileLoaderJob.finishLoading(JCSCachedTileLoaderJob.java:261)
	at org.openstreetmap.josm.data.cache.JCSCachedTileLoaderJob.run(JCSCachedTileLoaderJob.java:230)
	at java.base/java.lang.Thread.run(Thread.java:831)

[simon04]

Any idea what crashes the JVM in "Java 8 on windows-latest"?

[junitlauncher] # A fatal error has been detected by the Java Runtime Environment:
[junitlauncher] #
[junitlauncher] #  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x0000000067c29b1d, pid=6172, tid=0x0000000000000ef4
[junitlauncher] #
[junitlauncher] # JRE version: OpenJDK Runtime Environment (Zulu 8.52.0.23-CA-win64) (8.0_282-b08) (build 1.8.0_282-b08)
[junitlauncher] # Java VM: OpenJDK 64-Bit Server VM (25.282-b08 mixed mode windows-amd64 compressed oops)
[junitlauncher] # Problematic frame:
[junitlauncher] # V  [jvm.dll+0x129b1d]
[junitlauncher] #
[junitlauncher] # Core dump written. Default location: D:\a\josm\josm\hs_err_pid6172.mdmp
[junitlauncher] #
[junitlauncher] # An error report file with more information is saved as:
[junitlauncher] # D:\a\josm\josm\hs_err_pid6172.log
[junitlauncher] #
[junitlauncher] # If you would like to submit a bug report, please visit:
[junitlauncher] #   http://www.azulsystems.com/support/

https://github.com/simon04/josm/runs/2225967553#step:7:5214

[Don-vip]

Replying to simon04:

Any idea what crashes the JVM in "Java 8 on windows-latest"?

Probably this one:

• ​https://bugs.openjdk.java.net/browse/JDK-8179271
• ​https://bugs.openjdk.java.net/browse/JDK-8159956

It seems they never backported the fix to Java 8...

[Don-vip]

Workaround would be to call Toolkit.initIDs() in test code (using reflection), when tests are run on Windows, with Java 8 and headless mode.

[simon04]

Thank you, let's see: ​https://github.com/simon04/josm/commit/7184b2cb83b08309650277b9ecd41e2d97966683

[simon04]

In 17693/josm:

see #20621 - Fix JVM crash when running headless Java 8 tests on Windows

[simon04]

In 17694/josm:

fix #20621 - GitHub Actions: separate test and build jobs

[Stereo]

@stoecker 17693/josm means that your famous build script must be updated to look for the github job with the name "Java CI Build".

[simon04]

@stoecker, @Don-vip, are we ready for the milestone:21.03 release? I've just performed a (final?) i18n update, r17699.

@stoecker, have you updated the macOS linking script? Shall I revert the GitHub Actions name change from r17694?

[Don-vip]

Replying to simon04:

@stoecker, @Don-vip, are we ready for the milestone:21.03 release? I've just performed a (final?) i18n update, r17699.

Did you fix today's i18n issues first?

[simon04]

Replying to Don-vip:

Did you fix today's i18n issues first?

No, I haven't.

[skyper]

The three PMDs might be worth fixing, too.

[Don-vip]

I fixed the Dutch issues (typical missing quotes) but can't fix the Arabic ones. I asked for help in #18856. All i18n issues must be solved before we do a release.

[Stereo]

Just marking this as blocker to make sure the build script on the josm.openstreetmap.de re-runs the right macOS CI job by name.

[simon04]

I've fixed a bunch of ' errors in Italian:

     [exec] JAVA translation issue for language it: Mismatching single quotes:
     [exec] Translated text: Causato dall'attività umana
     [exec] Original text: Caused by human activity
     [exec] JAVA translation issue for language it: Mismatching single quotes:
     [exec] Translated text: L'incrocio non ha un nome
     [exec] Original text: Junction has no name
     [exec] JAVA translation issue for language it: Mismatching single quotes:
     [exec] Translated text: L'incrocio non ha un riferimento
     [exec] Original text: Junction has no reference
     [exec] JAVA translation issue for language it: Mismatching single quotes:
     [exec] Translated text: Modificato dall'uomo
     [exec] Original text: Modified by human hand
     [exec] JAVA translation issue for language it: Mismatching single quotes:
     [exec] Translated text: torre dell'orologio

The Arabic translations seem to rewrite the {x} with a corresponding spelled-out word:

     [exec] JAVA translation issue for language ar (3): Mismatching format entries:
     [exec] Translated text: تحديد {0}={1} لعنصرين
     [exec] Original text: Set {0}={1} for {2} object

"تحديد {0}={1} لعنصرين" = "Setting {0} = {1} of two objects"

I'd love to do a release without waiting for approx. 10 strings out of 9702 out of 41 languages to be fixed. Should I do a final i18n update now?

[Don-vip]

Replying to simon04:

The Arabic translations seem to rewrite the {x} with a corresponding spelled-out word:

I don't know what we should do about that. @Dirk, any idea?

[stoecker]

Replying to Don-vip:

Replying to simon04:

The Arabic translations seem to rewrite the {x} with a corresponding spelled-out word:

I don't know what we should do about that. @Dirk, any idea?

Not wanted, not supported. There are some limits to our i18n methods, that's one of them :-)

[stoecker]

P.S. I wouldn't delay a release because of these warnings ;-)

[simon04]

Very good. I'll do a final i18n update and then we're ready for the milestone:21.04 release...

@Dirk, what about comment:24? Will you update the script? Shall I revert the name change?

[stoecker]

@Dirk, what about comment:24? Will you update the script? Shall I revert the name change?

Already done. We'll see if it works.

[simon04]

Perfect, thank you :)

[Don-vip]

Replying to stoecker:

P.S. I wouldn't delay a release because of these warnings ;-)

I thought it might provoke a crash when using Arabic if we include these erroneous strings? What's the behaviour then, the string is just not translated?

[simon04]

No crash :-)

test/unit/org/openstreetmap/josm/tools/I18nTest.java

@@ -19 +19 @@
         assertEquals("'{'foo''bar'}'", I18n.escape(foobar));
         assertEquals(foobar, I18n.tr(I18n.escape(foobar)));
     }
+
+    @Test
+    void testAr() {
+        I18n.set("ar");
+        for (int i = 0; i < 20; i++) {
+            String trn = I18n.trn("Set {0}={1} for {2} object", "Set {0}={1} for {2} objects", i, "foo", "bar", i);
+            System.out.println(trn);
+        }
+    }
 }

تحديد foo=bar للا عنصر
تحديد foo=bar لعنصر واحد
تحديد foo=bar لعنصرين
تحديد foo=bar لـ ٣ عناصر
تحديد foo=bar لـ ٤ عناصر
تحديد foo=bar لـ ٥ عناصر
تحديد foo=bar لـ ٦ عناصر
تحديد foo=bar لـ ٧ عناصر
تحديد foo=bar لـ ٨ عناصر
تحديد foo=bar لـ ٩ عناصر
تحديد foo=bar لـ ١٠ عناصر
تحديد foo=bar لـ ١١ عنصرًا
تحديد foo=bar لـ ١٢ عنصرًا
تحديد foo=bar لـ ١٣ عنصرًا
تحديد foo=bar لـ ١٤ عنصرًا
تحديد foo=bar لـ ١٥ عنصرًا
تحديد foo=bar لـ ١٦ عنصرًا
تحديد foo=bar لـ ١٧ عنصرًا
تحديد foo=bar لـ ١٨ عنصرًا
تحديد foo=bar لـ ١٩ عنصرًا

Process finished with exit code 0

[Don-vip]

OK perfect! Sorry, I really thought it was a release-blocker problem, glad to see I was wrong.

[stoecker]

Replying to Don-vip:

Replying to stoecker:

P.S. I wouldn't delay a release because of these warnings ;-)

I thought it might provoke a crash when using Arabic if we include these erroneous strings? What's the behaviour then, the string is just not translated?

As the strings are valid in principle nothing special will happen (it's only the validity monitoring which cannot handle these cases and I don't plan to change that, we can't allow individual rules for each and everyone :-). Otherwise for broken strings the translation would result in strange results depending on the type of error.

[Don-vip]

Replying to stoecker:

As the strings are valid in principle nothing special will happen (it's only the validity monitoring which cannot handle these cases and I don't plan to change that, we can't allow individual rules for each and everyone :-). Otherwise for broken strings the translation would result in strange results depending on the type of error.

But we can't let things as it is now: it becomes impossible to spot the other problems, and I would like to get Jenkins job to go back to stable: https://josm.openstreetmap.de/jenkins/job/JOSM-i18n/2071/parsed_console/

[stoecker]

Replying to Don-vip:

Replying to stoecker:

As the strings are valid in principle nothing special will happen (it's only the validity monitoring which cannot handle these cases and I don't plan to change that, we can't allow individual rules for each and everyone :-). Otherwise for broken strings the translation would result in strange results depending on the type of error.

But we can't let things as it is now: it becomes impossible to spot the other problems, and I would like to get Jenkins job to go back to stable: https://josm.openstreetmap.de/jenkins/job/JOSM-i18n/2071/parsed_console/

Correct. The entries must be fixed accordingly, but it's not critical.

[Don-vip]

Replying to stoecker:

Correct. The entries must be fixed accordingly, but it's not critical.

OK, but how? :D I'm not sure to understand the problem fully, but Arabic has a special plural rule which means "two"? How can we add the {} parameter for this plural form?

[Don-vip]

r17702 is now the new tested but it doesn't seem to tag latest GitHub release.

[simon04]

​https://www.githubstatus.com/

• Update - GitHub Actions is experiencing degraded performance. We are still investigating and will provide an update when we have one. Apr 1, 21:36 UTC
• Investigating - We are investigating reports of degraded performance for GitHub Packages. Apr 1, 21:30 UTC

[Don-vip]

I think it just runs in nightly cron at 3am. Will check tomorrow morning, good night!

[simon04]

Good Morning, yay, it's released on GitHub: ​https://github.com/openstreetmap/josm/releases/tag/17702-tested

[stoecker]

Replying to Don-vip:

I think it just runs in nightly cron at 3am. Will check tomorrow morning, good night!

No. It simply needs extremely long :-)

[Don-vip]

ok!

[stoecker]

It seems GitHub CI for Pull Requests is used for Crypto Mining malware.
​https://www.heise.de/news/Cyberkriminelle-nutzen-GitHub-Actions-zum-Schuerfen-von-Kryptogeld-6005322.html