Modify

Opened 4 years ago

Closed 4 years ago

Last modified 4 years 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 by Stereo, 4 years ago

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 by simon04, 4 years ago

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 by Stereo, 4 years ago

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 by simon04, 4 years ago

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 by Stereo, 4 years ago

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 by simon04, 4 years ago

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

in reply to:  6 comment:7 by Don-vip, 4 years ago

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 by Don-vip, 4 years ago

Milestone: 21.03

comment:9 by simon04, 4 years ago

@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 by simon04, 4 years ago

Milestone: 21.0321.04

comment:11 by stoecker, 4 years ago

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

comment:12 by simon04, 4 years ago

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 by Stereo, 4 years ago

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 by simon04, 4 years ago

In 17690/josm:

see #20621 - GitHub Actions: formatting for readability

comment:15 by simon04, 4 years ago

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 by simon04, 4 years ago

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

Last edited 4 years ago by simon04 (previous) (diff)

comment:17 by simon04, 4 years ago

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

in reply to:  17 comment:18 by Don-vip, 4 years ago

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 by Don-vip, 4 years ago

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 by simon04, 4 years ago

Milestone: 21.0421.03

comment:22 by simon04, 4 years ago

In 17693/josm:

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

comment:23 by simon04, 4 years ago

Resolution: fixed
Status: newclosed

In 17694/josm:

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

comment:24 by Stereo, 4 years ago

@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 by simon04, 4 years ago

@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?

in reply to:  25 ; comment:26 by Don-vip, 4 years ago

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?

in reply to:  26 comment:27 by simon04, 4 years ago

Replying to Don-vip:

Did you fix today's i18n issues first?

No, I haven't.

comment:28 by skyper, 4 years ago

The three PMDs might be worth fixing, too.

Last edited 4 years ago by skyper (previous) (diff)

comment:29 by Don-vip, 4 years ago

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 by Stereo, 4 years ago

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 by simon04, 4 years ago

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?

in reply to:  31 ; comment:32 by Don-vip, 4 years ago

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?

in reply to:  32 comment:33 by stoecker, 4 years ago

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 by stoecker, 4 years ago

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

comment:35 by simon04, 4 years ago

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?

in reply to:  35 comment:36 by stoecker, 4 years ago

@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 by simon04, 4 years ago

Priority: blockernormal
Resolution: fixed
Status: reopenedclosed

Perfect, thank you :)

in reply to:  34 ; comment:38 by Don-vip, 4 years ago

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 by simon04, 4 years ago

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 by Don-vip, 4 years ago

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

in reply to:  38 ; comment:41 by stoecker, 4 years ago

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.

in reply to:  41 ; comment:42 by Don-vip, 4 years ago

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/

in reply to:  42 ; comment:43 by stoecker, 4 years ago

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.

in reply to:  43 comment:44 by Don-vip, 4 years ago

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 by Don-vip, 4 years ago

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

comment:46 by simon04, 4 years ago

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 by Don-vip, 4 years ago

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

comment:48 by simon04, 4 years ago

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

in reply to:  47 comment:49 by stoecker, 4 years ago

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 by Don-vip, 4 years ago

ok!

comment:51 by stoecker, 4 years ago

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. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.