Skip to content

Conversation

@sdedic
Copy link
Member

@sdedic sdedic commented Apr 16, 2021

I have changed the gavsplit, since versions provided in the artifact ID contain 4th component, the {timestamp}-{sequenceno} for Mavenrepo SNAPSHOT artifacts.

I changed the code to search from the start, as the 'official' syntax for artifact ID is group:artifact:version[:classifier][@ext] ... so looking for 3rd : delimiter seems to satisfy both this and group:artifact:version:timestamp-sequence which comes to gavSplit for Maven SNAPSHOTs.

@sdedic sdedic requested a review from lkishalmi April 16, 2021 08:55
* Checks GAV split with -SNAPSHOT and a maven-like timestamp/sequence as the
* snapshot unique id.
*/
public void testGavSplitFixedSnapshotWithMavenTimestamp() throws Exception {
Copy link

@JaroslavTulach JaroslavTulach Apr 16, 2021

Choose a reason for hiding this comment

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

Good! Btw. I think I had similar problem in Maven. When a really large project (bck2brwsr in my case) was just partially rebuilt, it started to download these dated artifacts from snapshot repository. Then classpath was full of dated artifacts and Bck2Brwsr had problems with it (as it takes JARs from classpath and generates .js files along them) - the names of JARs were suddenly different to what one normally expected.

@sdedic sdedic added kind:bug Bug report or fix Gradle [ci] enable "build tools" tests labels Apr 16, 2021
@sdedic sdedic self-assigned this Apr 16, 2021
@sdedic sdedic added this to the 12.4 milestone Apr 16, 2021
@sdedic
Copy link
Member Author

sdedic commented Apr 26, 2021

@lkishalmi does the change looks sane ? Please review/comment.

Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

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

Nice one!
Being out of the Maven world it is really easy to forget that the bad practice of SNAPSHOT versions are still a thing there...

@sdedic
Copy link
Member Author

sdedic commented Apr 26, 2021

@lkishalmi would you support including this one into delivery for 12.4 release ?

@lkishalmi lkishalmi changed the base branch from master to delivery April 26, 2021 20:13
@lkishalmi
Copy link
Contributor

Sure. The change itself is "trivial" enough and it has tests as well.

@geertjanw geertjanw merged commit f7b6fe7 into apache:delivery Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gradle [ci] enable "build tools" tests kind:bug Bug report or fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants