-
Notifications
You must be signed in to change notification settings - Fork 28
Deprecate HTML4-error-reporting methods/classes and update pom.xml for the 1.5 release #22
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
Open
sideshowbarker
wants to merge
9
commits into
release-1.5
Choose a base branch
from
release-1.5-with-deprecated-annotations
base: release-1.5
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
dc10ae9
Deprecate HTML4-error-reporting methods/classes
sideshowbarker 2d78654
POM: add an automatic module name entry to MANIFEST.MF.
carlosame e609391
Update xom dependency to 1.3.5+ in pom.xml
sideshowbarker bd686c6
Remove com.sun.tools dependency from pom.xml
sideshowbarker ed17453
Update source and target values to 1.7 in pom.xml
sideshowbarker a817dec
Update pom.xml for the 1.5 release
sideshowbarker d23c88f
Maven POM: set the OSGi RequiredExecutionEnvironment to 1.7
carlosame 2bab502
Maven POM: update the 'scm' information
carlosame 2b3dc35
Fix broken Java 8 maven build due to maven-antrun-plugin being unable…
carlosame File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Update xom dependency to 1.3.5+ in pom.xml
- Loading branch information
commit e609391c32120c842f42e2624158856cfb2f7049
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As explained in #17 (comment), I believe this should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the dependency upgrade (see #26)
However, I hold that the use of version ranges should be reverted, as it's not a common Maven practice. See here for a typical Maven example. As already mentioned here, I believe fixed versions should be used, among other things "for the sake of build reproducibility and stability".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is perfectly common and I use them after feedback from my downstream users, which may not always get the versions that they want if I do not do that. In fact, in the "typical Maven example" that you link, they say:
These dependencies should be aligned with the ones from the WildFly version we supportSo they use these exact dependencies as the result of an upstream decision.
I do not have a strong opinion against using single versions, but the version ranges are self-explanatory about the supported versions and (my) downstream users prefer that.
Maven builds are not reproducible unless you use the
reproducible-build-maven-plugin(which I do have in some projects), and even then the result may vary. And if some newer version of a dependency breaks the build, that's something that I'd personally prefer to know sooner (at upstream build/deploy time) rather than later (when downstream hits it).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, "common" was a bad choice of words. Let me state it differently: I very rarely, if ever, come across Maven POMs that use version ranges for dependencies.
From a quick look at some of the most popular Java libraries/frameworks on GitHub that use Maven, I don't find any that use version ranges for dependencies: Maven itself, Guava, Byte Buddy, Spring Data JPA, Hibernate Validator, commons-lang, Quarkus, Helidon, ... (same holds for a few Gradle projects I looked at, such as Spring Framework)
Guava has nearly 197000 dependents. Now I don't know how many of those use Maven, but if the lack of version ranges would really pose a problem for downstream consumers, I'm pretty sure Guava would be using them by now.
By using fixed versions, you always know for each commit which version was used for each dependency: so if it built 2 years ago, it'll still build now. And if 2 developers clone the repo and build any given commit, they're always going to have equivalent results w.r.t. dependencies.
And w.r.t. downstream users: say 2 years ago
LibA 2.1was published, with a dependency onLibBdeclared as[1.7.1,).Now if today I declare
LibA 2.1in my POM, and the build fails becauseLibBis at version6.5.8, which is incompatible withLibA 2.1, then what do I do? At this point, all I know is: "at least some version >= 1.7.1 that was available when LibA was published, is compatible". And since Maven can resolve very surprising versions, such as alpha/beta releases (there's even an open bug about Maven resolving SNAPSHOT versions for non-SNAPSHOT bounds), it's even possible thatLibA 2.1is not compatible with any released version of LibB.On the other hand, if LibA would've used fixed versions, things would just work.
As for detecting compatibility issues with new versions of dependencies: I agree that it's better to detect this sooner rather than later, but I believe there are better ways to do so.
If we consider a downstream user again: if
htmlparser 1.5declaresXOM [1.3.5,), then 6 months from now, some downstream user might file a bug sayinghtmlparser 1.5is not compatible withXOM 1.3.6. And then the burden would be onhtmlparserto fix the issue, because it basically declared "any version >= 1.3.5 works". On the other hand, by declaringXOM 1.3.5, a downstream user might still file a bug, but it's clear thathtmlparseris not at fault, and never claimed it was compatible with any version > 1.3.5.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was told otherwise by a former Maven developer, although perhaps there was a misunderstanding. And very large projects are much less likely to use dependency ranges.
Maven dependency version resolution is supposed to be somewhat flexible, and I'm not sure under which circumstances the problem would happen, but I was told that the safe approach (in a complex environment where fixed versions of the same dependency were specified by multiple libraries) was to configure exclusions. That may affect very big projects only though, and I haven't really seen a detailed account of it.
A dependency range does not give less information than a fixed one. If you know that the minimum version is
1.7.1, that means you can build with exactly that one. Otherwise, the range is wrong.And such a resolution problem would affect the upstream developer, not downstream. If a beta is not compatible, that's a bug that you may have to file.
In css4j I have detected some important upstream issues by this method. BTW this requires a significant amount of tests that are able to detect such problems (about 45% of the css4j code is test-related).
That's the other part of the issue: in my opinion, users have the right expectation that a supported project is going to run with reasonably recent dependencies, and if that is not the case they sometimes may file a bug with the dependency, sometimes with you. I have had a few of those, and have to deal with that as a maintainer.
This project can choose to depend on a fixed version (and some people are going to understand that the library is endorsing that version, or can only work with it), or specify a range (and other people are going to have ridiculous expectations about what that means, as you mention).
The range could be more conservative than what I suggested: for example
[1.3.5,2)could be safer for XOM. I specified a range for two of the dependencies (XOM and ICU4J, no plan to do the same with others), and these are unlikely to cause problems in the future. ICU4J numbering in particular is growing fast (currently at67.1in Maven Central).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally found out what was going on with the fixed dependencies; it appears that for downstream users, setting this:
<version>1.3.5</version>is effectively the same as:
<version>[1.3.5,)</version>which is not what I read in:
https://maven.apache.org/pom.html#dependency-version-requirement-specification
"
1.0: Soft requirement for 1.0. Use 1.0 if no other version appears earlier in the dependency tree."nor in:
https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#transitive-dependencies
"Maven picks the "nearest definition". That is, it uses the version of the closest dependency to your project in the tree of dependencies. You can always guarantee a version by declaring it explicitly in your project's POM. Note that if two dependency versions are at the same depth in the dependency tree, the first declaration wins."
Nope, the real behaviour is described here:
https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#CJHDEHAB
"
1.0| It generally means 1.0 or a later version, if 1.0 is not available. Various Maven plug-ins may interpret this differently, so it is safer to use one of the other, more specific options."That is, Oracle recommends using version ranges...
More from that page:
"When Maven encounters multiple matches for a version reference, it uses the highest matching version. Generally, version references should be only as specific as required so that Maven is free to choose a new version of dependencies where appropriate"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marbaise says on the same page:
I hope we can agree that (a) this is not the point of view you're defending here, and (b) given Marbaise's Maven expertise, it's much more likely that his view is in line with best practices.
I rest my case. It is beyond me why we're still having this discussion, if it's impossible to readily give me a list of projects that use version ranges and are at least somewhat popular. And just to give some verifiable counterexamples w.r.t. application servers: from a quick look, neither WildFly nor Payara use version ranges.
Which, as mentioned above, is not Marbaise's point of view. And "are highly proficient" is an unverifiable opinion, whereas it's trivial to verify that Marbaise is a Maven expert.
I'd say this speaks for itself.
With that being said, I'm out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that was before I read your latest comment. Honestly, I'm mindblown by your ability to select and interpret information in such a way that it matches your beliefs.
That page is about Maven in the context of Oracle Fusion Middleware. And yes, in that context, it's reasonable to declare a dependency as
[12.1.2,12.1.3), because you don't want to have to bump the dependency version whenever the middleware server is patched. This is about using strict version ranges for a very specific use case in a tightly-controlled environment.Also note that this is about the use of version ranges in middleware applications, not libraries.
As for the first part of your comment: if you believe it presents a valid point, please describe a concrete example (like: "A has a dependency on B, and B has a dependency on C. If A's
dependenciessection declares ... and B'sdependenciessection declares ..., then changing the section(s) as follows ..., will give equivalent results when doing a build of A"), or better yet: set up a repo with 3 folders A/B/C, each containing a Maven project, so I can see for myself what you mean.So let me try that again: with that being said, I'm out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mr. @anthonyvdotbe is "out" (and hopefully won't come back) and I'm tired of this thread as well, but if anyone wants to know about how those build failures (in complex projects) happen, there is an informative blog post here:
https://ourcraft.wordpress.com/2016/08/22/how-to-read-maven-enforcer-plugins-requireupperbounddeps-rule-failure-report/
My downstream project's build error was of the kind described there, and version ranges are an easy solution to the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarize the thread so far:
Given the following dependency graph:
Then when
root's POM declares:htmlparseris declared first orxomis excluded fromlibB: PASS with 1.3.5libBis declared first orxomis excluded fromhtmlparser: PASS with 1.3.6However, when enabling this rule from the
maven-enforcer-plugin, case 1.1 and 2 would FAIL instead.Now @carlosame holds that
htmlparsershould depend onxom [1.3.5,), since it allows case 1.1 to PASS, even with the rule enabled. However, I hold that version ranges should be avoided, and that the proper solution is forrootto modify its POM to match 1.2. or 3 instead.