Modify

Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#20621 closed defect (fixed)

GitHub pull requests should trigger CI

Reported by: simon04 Owned by: simon04
Priority: normal Milestone: 21.03
Component: Git mirror Version:
Keywords: template_report regression ci Cc: Stereo, Don-vip

Description

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

Attachments (0)

Change History (51)

comment:1 Changed 14 months ago by 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.

comment:2 Changed 14 months ago by 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).

comment:3 Changed 14 months ago by 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?

comment:4 Changed 14 months ago by 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.)

comment:5 Changed 14 months ago by 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.

comment:6 Changed 14 months ago by simon04

Cc: Don-vip added
Keywords: regression ci added
Type: enhancementdefect

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

comment:7 in reply to:  6 Changed 14 months ago by 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 :)

comment:8 Changed 14 months ago by Don-vip

Milestone: 21.03

comment:9 Changed 14 months ago by 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)

comment:10 Changed 14 months ago by simon04

Milestone: 21.0321.04

comment:11 Changed 14 months ago by stoecker

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

comment:12 Changed 14 months ago by 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

comment:13 Changed 14 months ago by 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.

comment:14 Changed 14 months ago by simon04

In 17690/josm:

see #20621 - GitHub Actions: formatting for readability

comment:15 Changed 14 months ago by 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.

comment:16 Changed 14 months ago by simon04

Any idea why DownloadWmsAlongTrackActionTest.testTMSLayer fails on GitHub Actions? -> #20695

Last edited 14 months ago by simon04 (previous) (diff)

comment:17 Changed 14 months ago by 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

comment:18 in reply to:  17 Changed 14 months ago by Don-vip

Replying to simon04:

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

Probably this one:

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

comment:19 Changed 14 months ago by 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.

comment:21 Changed 14 months ago by simon04

Milestone: 21.0421.03

comment:22 Changed 14 months ago by simon04

In 17693/josm:

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

comment:23 Changed 14 months ago by simon04

Resolution: fixed
Status: newclosed

In 17694/josm:

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

comment:24 Changed 14 months ago by 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".

comment:25 Changed 14 months ago by 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?

comment:26 in reply to:  25 ; Changed 14 months ago by 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?

comment:27 in reply to:  26 Changed 14 months ago by simon04

Replying to Don-vip:

Did you fix today's i18n issues first?

No, I haven't.

comment:28 Changed 14 months ago by skyper

The three PMDs might be worth fixing, too.

Last edited 14 months ago by skyper (previous) (diff)

comment:29 Changed 14 months ago by 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.

comment:30 Changed 14 months ago by Stereo

Priority: normalblocker
Resolution: fixed
Status: closedreopened

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.

comment:31 Changed 14 months ago by 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?

comment:32 in reply to:  31 ; Changed 14 months ago by 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?

comment:33 in reply to:  32 Changed 14 months ago by 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 :-)

comment:34 Changed 14 months ago by stoecker

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

comment:35 Changed 14 months ago by 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?

comment:36 in reply to:  35 Changed 14 months ago by 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.

comment:37 Changed 14 months ago by simon04

Priority: blockernormal
Resolution: fixed
Status: reopenedclosed

Perfect, thank you :)

comment:38 in reply to:  34 ; Changed 14 months ago by 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?

comment:39 Changed 14 months ago by simon04

No crash :-)

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

    diff --git a/test/unit/org/openstreetmap/josm/tools/I18nTest.java b/test/unit/org/openstreetmap/josm/tools/I18nTest.java
    index 4ac87849e..9e5e28f4d 100644
    a b class I18nTest { 
    1919        assertEquals("'{'foo''bar'}'", I18n.escape(foobar));
    2020        assertEquals(foobar, I18n.tr(I18n.escape(foobar)));
    2121    }
     22
     23    @Test
     24    void testAr() {
     25        I18n.set("ar");
     26        for (int i = 0; i < 20; i++) {
     27            String trn = I18n.trn("Set {0}={1} for {2} object", "Set {0}={1} for {2} objects", i, "foo", "bar", i);
     28            System.out.println(trn);
     29        }
     30    }
    2231}
تحديد 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

comment:40 Changed 14 months ago by Don-vip

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

comment:41 in reply to:  38 ; Changed 14 months ago by 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.

comment:42 in reply to:  41 ; Changed 14 months ago by 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/

comment:43 in reply to:  42 ; Changed 14 months ago by 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.

comment:44 in reply to:  43 Changed 14 months ago by 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?

comment:45 Changed 14 months ago by Don-vip

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

comment:46 Changed 14 months ago by 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

comment:47 Changed 14 months ago by Don-vip

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

comment:48 Changed 14 months ago by simon04

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

comment:49 in reply to:  47 Changed 14 months ago by 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 :-)

comment:50 Changed 14 months ago by Don-vip

ok!

comment:51 Changed 14 months ago by 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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain simon04.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.