XALANJ-2709 Enable project to be built on JDK 9+ #124
Conversation
|
Should be ready for rebase. |
The internal taglet module now exists in two variants for JDK 8 and 9+. Which one is included in the build depends on auto-activated profiles. This also works nicely in IntelliJ IDEA, it recognises profile activation and only builds the module corresponding to the current JDK, not yielding any build errors. I did not test on Eclipse, but hope that it works there, too. Either way, a stable Maven build is most important. I also streamlined the use of Maven Compiler Plugin.
kriegaex
force-pushed
the
XALANJ-2709
branch
from
bcf1dbf to
ca57656
Compare
|
Done, please review and merge. |
|
Throwing in a bit more opinionated talk: I dislike the practice in some OSS projects to squash multiple commits into one when merging. Firstly, if anyone looks at the merge commit, the commit message is huge and so is the code. The formerly logical separation of concerns in small commits is destroyed and therefore also the author's (or authors', if multiple developers contributed) intent. It is just like intended information hiding and defies everything I have taught to dozens of developers in Git classes. Small commits are good. Squashing them unnecessarily means one big chunk of code to scroll through and process in your mind. It also makes searching for errors much harder later. If I can use Moreover, it makes life harder for developers who manage multiple PRs depending on each other. In this example, if you would just have fast-forwarded my previous PR into the target branch, I would not have had to rebase, merge and force push, but this PR would just have been ready to merge as is. Please discuss this policy with your fellow Xalan developers. The practice is harmful, outdated, has no benefit whatsoever and tons of disadvantages. I could write more paragraphs, even chapters. P.S.: Sometimes squashing commits makes sense, e.g. when fixing a typo like the one I had in the POM with the forgotten "$" character. But then, I would squash the two related commits only, not the whole branch. |
Interesting thought. I can bring it up for discussion. It would mean changing a practice that dates back to when we first put this code onto Git, I think, so there's inertia as well as personal taste/experience in the mix. Some people are violently offended if we don't squish, though I don't think any of the current committers is that dogmatic. Personally, I lean toward merging small changes frequently, so a squish is usually minor and just eliminates some of the typo/thinko corrections or intermediate checkpoints that aren't intended to be shared. This Maven switchover is a major exception since, like an architectural change, it has a large all-or-nothing element to it -- or at least I didn't see a good way to make it incremental at the time. The early history of the Maven cut-over is squished largely because we got a demand to clearly separate the files which were just being moved from the files that were being changed/replaced. I decided to comply rather than argue about it, and I had to collapse it to achieve that resequencing. I can understand the ask, though I think the need was overstated, especially as the arguably easier review enabled by that separation doesn't actually seem to have produced much additional review feedback. I see the point about fast-forward. That may be the most convincing argument. In any case: Normally I'd only squish upon merge into the main stream. Which should normally be after regression testing has been passed, unlike the individual commits. The more serious continuous integration system we had running for Websphere Liberty still worked in terms of PRs rather than commits, since that was the level at which automatically backing out interacting or otherwise suspicious changes was practical. Backing out individual commit points would have left the system between stable versions, since non-squashed commits are not promised to be complete enough or isolated enough to accept individually. Thanks for raising it; I need to think about it more. Y'know, that sort of thing really ought to be going onto the Xalan development mailing list rather than here, since it isn't especially unique to this changeset. That's also the easiest way to get it in front of all the developers. (You do realize that if you continue being active and helpful, you're at risk of us trying to pull you into the Xalan project, right?) |
@kriegaex ,
@jkesselm , please re-read what @kriegaex says. There are several very valid reasons to keep commits like in #122 separate, and I 100% agree with @kriegaex that structuring PR122 as five independent commits, and merging it as 5 independent commits was the best idea possible. Take this as an example
Imagine I upgrade Xalan in my enterprise app, and imagine it produces a weird NullPointerException after the upgrade. Since PR122 was merged in a single commit, the best I would get is "ok, commit 4af37e7 modified multiple unrelated files which introduced the NPE". If the commits were individual, then Of course, backing out a single commit does not guarantee the system would work, however, having five commits would simplify the analysis and review a lot.
I think you refer to #105 (comment), however, it seems you thought the key reason was to simplify Maven PR review. Let me clarify: I never cared reviewing Maven scripts as those are a waste of time. Later, in my comment I highlighted other reasons to split "file move" into a separate commit:
Let me remind you that the effort we were talking about was 15 minutes, so "overstated" does not sound right, especially since the benefits of separating "file move" extend far beyond "merging the PR". The benefits are for everybody analyzing Xalan failures later in their systems, and for those who would maintain Xalan.
That is not relevant. If you were to say "all logical contributions must be filed as separate PRs", then just say so. I can't understand how the mention of Websphere Liberty relates to PR122.
Well, fixing the build system for Xalan is a couple of days activity. I would be surprised if one could produce "much review feedback" out of that. There's just nothing to review. |
| <profiles> | ||
| <profile> | ||
| <id>jdk8</id> | ||
| <activation> | ||
| <jdk>[,9)</jdk> | ||
| </activation> | ||
| <properties> | ||
| <xalan.taglet.artifactId>xalan2jtaglet</xalan.taglet.artifactId> | ||
| </properties> | ||
| <modules> | ||
| <module>xalan2jtaglet</module> | ||
| </modules> | ||
| </profile> | ||
| <profile> | ||
| <id>jdk9+</id> | ||
| <activation> | ||
| <jdk>[9,)</jdk> | ||
| </activation> | ||
| <properties> | ||
| <xalan.taglet.artifactId>xalan2jtaglet_jdk9</xalan.taglet.artifactId> | ||
| </properties> | ||
| <modules> | ||
| <module>xalan2jtaglet_jdk9</module> | ||
| </modules> | ||
| </profile> | ||
| </profiles> |
There was a problem hiding this comment.
Even though adding two profiles is not much code, I suggest just requiring Java 17 for build, and use --release 8, --target 8 to target Java 8 bytecode. It would not require profiles, it would be easier to understand.
It would be way easier to support, and it would be way fewer bugs since javac 17 should be tested way better than javac 8.
There was a problem hiding this comment.
I had discussed that with @kubycsolutions in #120 (comment).
I said:
Another question is if you really want to force every developer to run the build on JDK 8 just because of the taglet dependencies on old
com.sunJDK packages, which have been superseded by others on JDK 9+. Basically, you have 3 options:
- Continue as is.
- Require JDK 9+ for the build and migrate the taglet thingy to the new API standard.
- Add a JDK 9+ variant of the taglet code on top of the JDK 8 version and configure the build to use one on JDK 8 and the other on JDK 9+.
To which Joseph replied:
Xalan currently promises to run on JRE 8. That doesn't absolutely require that we build on JDK 8, I suppose, but I want to retain the ability to do so. For now, that rules out requiring 9+. But we'll move forward someday.
As a first-time contributor to this project, I complied, which is the reason why the solution looks like this and no different. Vladimir @vlsi, can you please discuss with Joseph? My suggestion is to keep the build on JDK 8 for now, i.e. accept my solution, because all the work is done already, and I would be quite disappointed to have invested my time just to revert most of it again now.
There was a problem hiding this comment.
My take is that dropping JDK8 build compatibility is a separate discussion, worth considering but a bit of a distraction at this moment.
Changing the Java 8 compilation requirement -- if I'm not hallucinating that the policy still exists; remember I'm just getting back to Xalan 15 years later and still catching up -- is something I'd want to warn our users of. If nothing else, supporting both gives us a deprecation window.
So my take is: Valid question, open it separately as technical debt, discuss, and deal with it as its own issue rather than here.
Other Committers may have other opinions. But I'm going to accept this into my branch; it can always be simplified after that discussion, or folks can throw rocks at it before merge into main if they really feel we need to delay the already-large mvn proposal further.
(FWIW, I'm inclined to withdraw my PR to Master/Main again. It was originally posted for discussion; it wasn't ever ready for merge and in hindsight I should have just asked folks to review it unsquashed on the fork. And in fact once we have it ready to consider merging, I'm tempted to reconstruct it in proper incremental change form, thus undoing the squash and making the actual concepts clearer in the history, at the admitted cost of losing some history of my mistakes. Downside is my being distracted by it that much longer. I freely admit I mis-sized the effort to learn the Maven Way.)
There was a problem hiding this comment.
My take is that dropping JDK8 build compatibility is a separate discussion
Agreed.
Changing the Java 8 compilation requirement -- if I'm not hallucinating that the policy still exists
If it still exists, it is a bit weird. Java 8 is no longer supported, even as an LTS release. I understand that Xalan should run on Java 8, but where is the value in building it on Java 8? Anyway, I implemented it that way and hope you can merge it soon. Sorry for the pressure, but I always try to minimise the difference between touch time and cycle time in everything I do. Frequent context changes lead to loss of efficiency. Moreover, code rots.
If nothing else, supporting both gives us a deprecation window.
True.
(FWIW, I'm inclined to withdraw my PR to Master/Main again. It was originally posted for discussion; it wasn't ever ready for merge
Simply mark it as a draft PR.
once we have it ready to consider merging, I'm tempted to reconstruct it in proper incremental change form
That would be a total waste of time. It's what it is. (Sorry, I watched "The Irishman", could not resist.) That PR was open too long already. I suggest focusing on getting it done, reviewed and merged. It does not need to be perfect, just good enough. Besides, perfection would be in stark contrast to the rest of the code in Xalan-J. 😉 IMO, the sooner we can leave Ant behind, the better.
Moreover, I would not like to see my commits being redone by someone else who is then listed as an author.
While I agree that resource starvation is a common problem in many OSS projects, it is not my point here. Even if in a very actively managed project someone reviews PRs on a daily basis, a contributor might, just like I did, work on issue A, noticing B and prepare multiple PRs in short order, e.g. the same day. Rebasing PRs is always a requirement, and it is eased by rebasing on a commit history without merge commits, hence the idea to fast-forward them.
Thanks for supporting me in this regard.
@kubycsolutions / @jkesselm (which of the two user names do you prefer to me mentioned by?), Vladimir understood correctly. I suggested to never force-squash PRs into single commits, because in most cases users would bisect problems in a release version, not in a random development version. Releases, however, are built from the main branch, and there again would be the clunky squashed commits. Vladimir explained it very well in his example use case.
Sorry to ask off topic, but as you brought it up: Why would reviewing Maven scripts be a waste of time? They influence, even define the whole build process. Any change can mess up the build. Reviewing them is well invested time.
Yes. The main motivation should be separation of concerns. Imagine you mix functional changes with structural refactoring and major reformatting. Now you make one big commit. When the next developer or user scrolls through a |
|
Checking this PR today. Thanks yet again for all the work you've put into helping me understand Maven! I agree that my squash earlier in this branch was badly premature, and in retrospect I agree that Vlad's proposal was a decent way to rescue that. Once he pointed me to how to extract the information about what had been only-moved, moved-and-changed, or created from scratch I found a way to automate the separation of commits -- this time with Emacs macros running against the list, but it could be coded. But I needed help finding first step; without it the task looked excessively expensive. I think you're convincing me that squash-and-merge also has significant drawbacks, at least in this environment; in past projects having atomic merges seemed to be a win. I'll try to change my usage pattern to reflect that. Feel free to call me on it if I forget. |
|
Looks good from here on openjdk 1.8. I'll load up latest and sanity-check, but I don't anticipate any problems. |
|
@jkesselm |
It was just a historical practice that you learned and a convention, albeit dated, which still is in effect in too many projects. It is not like you committed a crime or anything. We hang around, we learn. 🙂
In CVS and SVN, commits and branch switches were expensive and merges painful. That is where this practice might partially come from. Developers wanted to avoid that pain by avoiding feature branches, committing infrequently and merging even more infrequently. Combine that, and you get a preference for squashed merges. In Git, it is different: All operations except for fetch and push can be done offline without any network connection. I.e., commit, branch switch, merge are blazingly fast. In SVN, all of these require network connections and bandwidth. If SVN, a merge basically is rooted in a rather naive diff between two directory trees, while Git retains a DAG (directed acyclic graph) of the commit, branch, merge history, which is why several types of merge conflicts you would encounter in SVN simply do not occur, e.g. if you re-merge the same branch multiple times. Last but not least, even bisect is a local operation and thus also fast. In SVN you can bisect logically, if you want, but each checkout requires loading tons of data from a central server. I.e., using that error hunting strategy is way less enticing than with Git. Hence, nobody cared much about small commits in the old times. Been there, seen that. For me personally, back then, knowing SVN well and even administering it, for a few years I avoided using Git. Then one weekend, I got curious and started to read "Pro Git" by Scott Chacon, which nowadays is available in many languages and open source. I could not put the book down, and at the end of the weekend I had read it back to back, because the technical concept behind Git was so intriguing. It was all so simple, one had to wonder why nobody had come up with something like it before and SCM had not been like that forever. I immediately switched to Git in all my personal projects and never looked back. I established a practice of consequently using feature branches and small commits, and my life as a developer became so much better over night. As an agile coach, since then I have introduced Git to dozens of teams in several organisations. Despite initial resistance against feature branches and frequent merges due to fear caused by lack of knowledge, all developers embraced it, once they got the hang of it. IMO, agile software development - or let me just call it clean and professional software development - is next to unthinkable without feature branches for reasons which to explain in detail would make this post even more lengthy. In a nutshell, it is a cornerstone of (agile) software development that a team works on multiple user stories in parallel per sprint. There you have the need for feature branches. Whenever a story is done (i.e. acceptance-tested), it gets merged into the development branch and all other story branches need to refresh their branches from there (merge). The next story is done, the cycle starts again. You only ever have accepted work on the development branch. Eventually, e.g. at the end of each sprint or some other defined point in time, you cut a release, merging the development branch into the main branch. No work ever gets done on the development or main branches, even a one-line bugfix is done in a feature branch. Working like this, if done right, is no problem in Git. It can be done in SVN, too, but would be all but nightmarish in practice. |
I agree 90% with @garydgregory. 🙂 In this particular case, JDK 9 is the very version in which the build behaviour is supposed to change, i.e. it might be due diligence to check JDK 9 too, despite its not being an LTS release. With JDK 9 came Jigsaw (Java modules), i.e. it might come in handy to have a JDK 9 around on your hard drive, as long as it is on top of 11, 17, 21 and not the only JDK version greater than 8. |
@kriegaex , that was my point ;)
Based on my experience, Maven-based builds face limitations very soon, so I think investing time in many Maven builds is a waste. Here are some cases from Xalan:
Currently, Xalan does not have many third-party dependencies (which is nice), however, Maven has an inherent flaw when resolving conflicts among transitive dependency versions: https://issues.apache.org/jira/browse/MNG-7852 |
While I agree that Maven is not as dependency-driven as I would have expected/hoped it to be all these decades after But Javadoc/stylebook processing really is optional when not publishing for release. |
|
Can we please refrain from comparing Gradle to Maven? Some people love Gradle, others favour Maven, but that is not the point. We are in a Maven context here, therefore both Gradle advertising and Maven bashing are not solving any problem. Joe asked for help about Maven on the users mailing list, so I tried to lend a hand. If he had asked about Gradle, I would not be here. Vladimir, if you want to help with the Maven build, you are welcome. If you just want to spit into the soup and urge us to use Gradle instead, please stay away. Back to Maven: Maven is dependency-driven, but it revolves around dependendies between modules, not build tasks. Knowing that and knowing how to handle it, maybe throwing in a few flags and/or profiles, helps to solve most problems quite easily. Joe, the fact that you deliberately decided to create javadocs only in the |
@kriegaex , Joseph was sad Maven PR was not getting much feedback, and I replied I was not interested. You asked the reasons I was not interested so I replied. That is it.
Frankly, it produced quite negative a emotion. As I am far from speaking English natively,so it is hard to get the tone. Please consider that it was you who asked the reasons I did not want to review Maven scripts, and I replied. No way I am someone who randomly spams with "just use X instead of Y". |
No, you said it was a waste of time. That is a judgemental statement. So I got curious. I thought you said that, because you felt that reviewing build config as such was a waste of time in general. Only later, I found out that you probably would gladly review Gradle builds, just not Maven builds. And then, I got annoyed by your statements both on GitHub and Jira, all in the gist of "Gradle is great, Maven is crap" (paraphrasing the vibe I got when reading them).
That was intentional. You got on my nerves with your negative, non-constructive statements, so I wanted to tell you about it in no uncertain terms, because not reviewing Maven POMs is a waste of time, but having to read utterly unhelpful statements like yours. @vlsi, you see that I am open for criticism, having taken your concerns about package relocation seriously and addressing them here, even though you bluntly just suggested to decline the PR. Hence my statement, that some people find a solution for every problem and others a problem in every solution. Being more constructive and less condescending in your own conversation style might do wonders to mine in response. Give it a try, you might be surprised. |
Please be civil. You asked me the reason I was not interested, I replied, and now you say you get annoyed by my statements. As you stress that "...build process. Any change can mess up the build. Reviewing them is well invested time", please check out #2 and my other PRs to xalan-java. They are all build-related.
Sure reviewing Maven scripts is a waste of time for me. Sure there might be people who adore reviewing Maven scripts. It does not mean "Maven is crap" or "Maven is good". It just means I do not like spending time on reviewing Maven scripts.
First, thank you for figuring out the way properties file can be relocated. I believe my comment on the possibility of shade/relocate issues was constructive as I suggested a relevant case that should be tested for your change. At the same time, your issue XALANJ-2710 did not include the motivation in the first place. It is surprising to see "feature X to be implemented as Y" with no motivation. Sure there must be some motivation behind every change since every change might introduce bugs. It would be great if you included the motivation for the changes the next time you create issues.
Frankly, why not both? You seem to assume that people should split into either of those categories, while in practice both skills "finding solutions" and "finding problems" are mandatory.
Frankly speaking, I find this statement utterly unhelpful. It looks like you accuse me of spamming with irrelevant comments while I am sure my comments were helpful. It would be really helpful (no joke) if you could explain which of my statements were "utterly unhelpful". Please be exact.
I am afraid you misinterpret. Sure everybody is free to have the fun they like. I was always open to help with migrating to Gradle. Sure people can ignore it, however, it is unfair of you to say I am not constructive. I explained the reasons I do not want spending my time on Maven, please just recognize it and stop accusing me of raising "Gradle vs Maven". You want Maven, fine. Just go for it. At the same time, please stop judging others if they happen to dislike Maven. @kriegaex , I suggest you to check the following:
I am sure all of it is constructive, and they are nowhere near "Gradle is great, Maven is crap" you mention. |
I was and still am civil. It should be permitted for me to speak just as frankly as you do. It is quite interesting to see that you are always ready to deal blows, but seem to be unwilling to take any in response. Like I said: Be nice to me, and I will be nice to you. Tit for tat. You were not nice, but repeatedly made snarky, even condescending comments like Maven being a "waste of time".
I am talking about our interaction here, not about whatever other PRs - build-related or not - you might have worked on before. They are utterly irrelevant here and in no way entitle you to make dismissive comments and then be surprised about confrontative responses. Again: tit for tat.
Then I suggest, that you choose your wording more carefully next time. Out of context, the comment is most likely to be understood as judgemental, snarky, condescending - especially, if you follow up both here and on Jira with Maven vs. Gradle comparisons. You do not need to "adore" Maven or the people using it. Just do not look down on them. You are in no way superior or more advanced, because you happen to prefer Gradle.
Which is why I thanked you, even burnt into the commit history forever. But your first comment on the Jira issue was to suggest to unconditionally decline and close it.
Do you even notice how condescending you treat other people? Try that with others, but not with me, I am not having it. Or live with the way I respond. I had to protest first, before you finally made more helpful comments, and only then this happened:
Then why don't you start with that kind of review next time? Skipping the rest of your comments, because my time box is up. I already regret answering this much, because I actually I do not want to read twice the amount as a response from you, feeling compelled to reply once more. Instead, I would like to continue helping Joe here and there with his Maven migration. You can either join us or let us work in peace. There is no need for you to "waste your time" with more Maven-related things. Helpful code reviews are always welcome, though. |
|
@vlsi, one more note, this one more personal: I am not sure what you think you need to prove to the world or what past experience you are trying to compensate. Everybody with half a brain can see that you are smart, even very smart. So am I, but so what? Being smart does not mean one has to be a smart ass. 😉 (Please look up the term, if you are unfamiliar with it, e.g. here.) I know it can be difficult to be intelligent and not to come across to others as arrogant. I have been accused of arrogance many times before, just because I said something "smart" in response to others, e.g. pointing out flaws in their reasoning or otherwise correcting mistakes. Some people mistake a quick response for a response without thinking, because they cannot imagine that somebody actually thinks that fast and the response includes eventualities they think you forgot to analyse, even though in fact you already have. Therefore, for people like us, just like for everyone else but even more so, it is important to show a certain degree of patience and never to forget that being right (or believing to be) is no excuse for being curt or unkind. OTOH, if anyone treats us like that, of course we should not simply accept that, which is why I am not accepting it from you. Try to be kind. Not only does it make you a happier person, but from a pure efficiency and also effectiveness point of view it often saves you tons of time, e.g. the time we have spent here already for our flame war, and gets you what you desire more quickly and easily (not always, but surprisingly often). |
@kriegaex , I never attacked you personally, while you attacked me intentionally. At least, if there was an attack, it was 100% unintentional, so it is no joke I ask you to pin-point the exact comments that you treat as attacks. The suggestion to "decline" is not a veto, and I do not see why take it personally. Sure I might have expressed it better, however, I believe even the first comment was actionable and reasonable. The suggestion to decline was related to PR rather than its author. I do not get why you take Maven's faults as if they were your faults. |
4 participants
Fixes https://issues.apache.org/jira/browse/XALANJ-2709.
@kubycsolutions, you can start reviewing and testing this PR. After you have merged #123, I can rebase this one on your then current Maven branch again and force-push to provide for a clean merge later on. Then, this PR will show fewer commits, i.e. only the ones I made on top of #123 (currently a single commit).