Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MSHARED-955] make parseOutputTimestamp static #10

Closed
wants to merge 1 commit into from
Closed

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Sep 1, 2020

@elharo elharo requested a review from hboutemy September 1, 2020 15:59
@pzygielo
Copy link

pzygielo commented Sep 1, 2020

It is IncompatibleClassChange (apache/maven-ear-plugin#10 (comment)).

@mabrarov
Copy link

mabrarov commented Sep 1, 2020

@pzygielo, sorry for a stupid question, but does it make sense if this change will be released with new version of Maven Archiver? Isn't it sufficient to notify users of this library about this incompatible class change in release notes so that they will become aware of it when migrating to the new version of Maven Archiver?

@pzygielo
Copy link

pzygielo commented Sep 1, 2020

Isn't it sufficient to notify users of this library about this incompatible class change

Maybe it is sufficient, but one has to be aware of that to prepare notification.

@elharo
Copy link
Contributor Author

elharo commented Sep 1, 2020

I think the issue only appears if a subclass overrides this method.

@pzygielo
Copy link

pzygielo commented Sep 1, 2020

I think the issue only appears if a subclass overrides this method.

If so changed class appears in place of old one - without recompiling client - then IncompatibleClassChangeError would be raised.1 Thus it depends how maven-archiver artifact is used.

  1. I imagine overriding maven-jar/ear-plugin dependency to use new release of maven-archiver for example.

@elharo
Copy link
Contributor Author

elharo commented Sep 1, 2020

@pzygielo can you provide more details on what code would be required to trigger this?

@pzygielo
Copy link

pzygielo commented Sep 1, 2020

@pzygielo can you provide more details on what code would be required to trigger this?

See https://github.com/pzrep/MSHARED-955

@elharo
Copy link
Contributor Author

elharo commented Sep 1, 2020

Thanks. That's not something I'm worried about happening in practice. Is there a likely diamond dependency problem where the incompatible version gets selected?

@pzygielo
Copy link

pzygielo commented Sep 1, 2020

That's not something I'm worried about happening in practice.

I know. But I do.

So I've updated my example for the case, where this dependency is overriden in assembly plugin (i.e. archiver is no longer direct dependency of code in my example, but is utilized by plugin).

Per http://maven.apache.org/plugins/maven-assembly-plugin/dependencies.html assembly plugin uses archiver in version 3.5.0 (the latest).

Per https://maven.apache.org/guides/mini/guide-configuring-plugins.html#Using_the_dependencies_Tag:

You could configure the dependencies of the Build plugins, commonly to use a more recent dependency
version.

So I do in my example, pretending that 3.5.1-SNAPSHOT is released now and I like other changes in it, and there is no better release of assembly plugin. Example shows that proposed change is not backward compatible.


I'm only saying, that if there is no other way - it shall be reflected in version of plugin, as per https://semver.org/, which might not be observed here but is kind of standard:

increment MAJOR version when you make incompatible API changes,

or at least as @mabrarov said - very clearly stated in RN.

@rfscholte
Copy link
Contributor

For the record: Maven doesn't really understand semver, it is just a hint. If for the same groupId+artifactId a version 1.0 and 3.1 are in the dependency graph, Maven will pick only 1 based on the "nearest wins"-rule. Marking breaking changes with the versions might work for frameworks like spring, but not for libraries like commons-lang. Hence they made the right decision to change both artifactId and the package to prevent collisions.

@mabrarov
Copy link

Is this PR still actual?

@michael-o
Copy link
Member

Likely, @hboutemy WDYT?

@elharo elharo closed this Jan 20, 2021
@michael-o michael-o deleted the MSHARED-955 branch June 22, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants