SUREFIRE-1588 Patch (Java7) #197
Conversation
Not perfect, as this uses Java7 Path ;) But it works OK and am fine with 2.22.1 compiled for Java7
As it uses Java7 API, while lang level and sniffer were set to Java6.
|
Hi @cstamas |
|
@Tibor17 Hi there! It might become necessary to make 2.22.2 bugfix release, as this seems to affect a LOT of people out there (seems this check is also in Oracle Java 1.8.0_191?) |
|
@cstamas |
|
@cstamas |
|
I will need this fix too. 2.22.2, please? |
|
+1 to have this released ASAP as it's really blocker. |
|
This issue is also affecting open jdk 8 and oracle jdk 8 |
|
Now the API 3.0 is applied. I am working on Java 1.7 and plexus-java@java7 in branch |
...rc/main/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfiguration.java
Outdated
Show resolved
Hide resolved
The classpath entries are expected to be relative URIs and not plain relative filepaths. Hence, special characters like whitespace and other should be properly encoded.
And log the fact, as nothing else came to my mind.
...rc/main/java/org/apache/maven/plugin/surefire/booterclient/JarManifestForkConfiguration.java
Outdated
Show resolved
Hide resolved
So constructing URI with 3 param URI ctor instead, and will fail in case of USEx.
|
|
Hi @rfscholte . The CLI in shared-utils executes a subprocess and we in surefire rely on a callback that the process finished. It seems we do not receive this notification and therefore the process waits next 10 seconds till timeout has elapsed. This prolonged ITs from 1 hour to 2 hours. Using the older version 3.1.0 all is same as before. I will have time to debug it after Surefire release. Are you fine with that yet? |
|
My guess: apache/maven-shared-utils@f2246e0 Would be nice if you could have a look at it once you've finished this phase of surefire. |
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Looks right. Is there a specific integration test that reproduces the issue when run on a new Java release?
|
I think it's worth trying with an integration test that uses a random port. |
|
@cstamas |
|
A new branch |
|
We should see the Manifest. Why this issue does not exist on Windows? I think it should because the code base is in TEMP since the |
|
@cstamas Can you please squash the commits in one? Exclude the |
| File file1 = new File( it.next() ); | ||
|
|
||
| String uri = parent.relativize( file1.toPath() ).toString(); | ||
| String uri = URI.create( parent.relativize( file1.toPath() ).toString() ).toASCIIString(); |
There was a problem hiding this comment.
Works for me but the annoying are logs because we log every Jar file.
This can be fixed by using a boolean flag preventing from repeative logs with same message.
Can you segregate the try-catch to a private method?
I have a question regarding the bug. The build plugin crashed with plugin version 2.22.1 on 1.8.0_192? It did not crash on my Windows system. It is different experience on Linux?
There was a problem hiding this comment.
AFAIK our users, they won't see any warnings. So. We should write the log down to a dump file, see InPluginProcessDumpSingleton. I do not think it is user error. You need to specify reports dir and pass it to the class because the InPluginProcessDumpSingleton requires the dir.
What does this even mean? We need a fixed release, and I don’t mind bumping the minimum Java version as everything here is Java 8 anyway (with one Java 7 project from another team), but what other incompatible changes will you introduce in version |
The issue is caused by a bad backport of some new security features from OpenJDK via JDK 10 to JDK 8 in Debian, by the Ubuntu-employed maintainer and the Debian security team. There are a couple new checks for JAR files, and one of them triggers the issue. According to someone who analysed the OpenJDK upstream changes, the OpenJDK team later disabled that new check by default, but this did not get backported to Debian. So I expect that OpenJDK itself will enable the new check some time in the future, at which point it will fail everywhere. For now, it only fails on Debian and derivatives, and it’s extremely unlucky that this change was forcefully pushed even onto stable release users prematurely, under the umbrella of security fixes and “a deliberate upstream change”. |
|
Are there any objections if I proceed with pushing the fix to master? @mirabilos |
I know that. I was just asking whether upgrading from 2.x to 3.x will break anything except Java 6. |
|
ok, I am just running a local build and I will push it to master in few minutes only and then I will start the release Vote. Hoping this helps. |
|
@cstamas |
|
Please ping back when the release (and which |
|
@mirabilos |
|
@mirabilos |
Ah thanks, I was trying to figure that out, but did not get as far. Yes, I can confirm that the snapshot version I’m a bit wary of major upgrades though, due to unwelcome surprises such as the still-unfixed |
|
@Tibor17 Tested 3.0.0-M1 on my machine, and it is OK. |
|
@mirabilos |
|
@cstamas I am inhurry with other appointments. Feel free to investigate. If you want to fix it, feel free, but please run the |
|
The problem is that |
|
IMPORTANT UPDATE: as I removed the logging from the PR locally (mention above), I also removed the report stuff from master, and then I got these IT results, so please mind me, I forgot to mention and defs don't want to mislead you or anyone else. Sorry again. @Tibor17 I ran locally ITs of the current master modified by removing line 152 in JarManifestForkConfiguration, the logDump call (on affected Linux with b13 openjdk8), and I got totally different outcome: none of those ITs you mention failed, but instead I have: And the most interesting part: some ITs failed due SUREFIRE-1588! For example, SUREFIRE-1588 causes org.apache.maven.surefire.its.SiblingAggregatorIT failure, produced traces like these: Please note, all 2.x surefire plugin versions are affected of SUREFIRE-1588, hence it failed (unsure why IT uses 2.12.4 in the first place and not the built plugin). Surefire162CharsetProviderIT fails due this And is right, there is no such thing. And finally the Surefire855 tests fails as they assert this So, it is there but as relative URI So all in all, I have no clue what is happening on master. In the mean time, I started ITs of this PR (so 2.22.1+this patch w/ logger removed). Will report back how that goes. |
As there is no simple way to notify user about this it seems, also, extra report makes some ITs go hayway.
|
Btw, second look: SiblingAggregatorIT POMs are not using Surefire162CharsetProviderIT copied the jcharset into wrong repo, this is why it did not found the artifact in local repo: it copied it to and finally, the Surefire1535TestNGParallelSuitesIT failure you got, don't know what is it about, but: seems you are on Windows and you have local repo and maven workdir on different drive letters. With affected JDK and this kind of Windows setup this PR will not work. Unlike UN*X, Windows due drive letter will not be able to "relativise" paths like these. |
So maybe we do need to fall back to copying classpath JARs into the Can the whole issue be bypassed by running the booter with an explicit |
|
@jglick re copying, then we open a total different "pandora" box, think about dep artifact GAVs with differrent G's but same As and Vs (for example I'd def look into other directions as well... This whole thing started due |
|
@cstamas |
|
Feedback for everybody. We are on the Slack ASF chat with @cstamas and we are fixing and testing old ITs one by one and the changes are being pushed to the branch "buildfix" on Surefire project, and @cstamas has his own branch, so we cooperate. Pls be patient while we are working on current issues. Thx |
|
@cstamas |
|
@cstamas |
|
@cstamas I was thinking about ways this behavior could be used to inject JARs into the system class loader from the booter (after 8 it is no longer a |
|
@jglick |
|
Running into some problems on Windows which I am digging into. For reference: the actual committed fixes |
|
@jglick |
Already done: #198 |
|
Resolve #2132 |
1 similar comment
|
Resolve #2132 |
9 participants
The
JarManifestForkConfiguration.javachanges that makes Surefire 2.22.1 work for me ;)Still, this is not a "real" solution, as surefire (for me unnkown reasons) is set to Java6. So, 2nd commit is laxing this, and am happy to use this "patched" surefire on my machine as my shop is Java8+.
Code is done against
surefire-2.22.1tag, but the PR is targeting master as there is no branch for 2.22.x?