Skip to content

Reformat Maven POM and assembly XML, no functional changes #120

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.

Sign up for GitHub

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

Jump to bottom
Closed
kriegaex wants to merge 1 commit into from
Closed

Reformat Maven POM and assembly XML, no functional changes #120

kriegaex wants to merge 1 commit into from

Conversation

@kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Nov 14, 2023

  • Get rid of mixed tabs and spaces, use 2 spaces for indentation uniformly instead.
  • Use line width 120 with very few exceptions.
  • Use a uniform way to format multi-line XML comments, putting opening and closing tag parts on separate lines, indenting the text content by 2 spaces.

This can subsequently be the baseline for further commits, both by @kubycsolutions and external PR contributors.

Sorry for maybe seeming nitpicky, but when scrolling through the POMs in my IDE, it seemed somewhat messy and hard to parse in my limited brain. It might be a personal preference only, but I do like uniform indentation. As the Java code mostly seems to use 2 spaces and the POMs also partly followed that pattern, I chode 2 spaces too - maybe also, because that is my personal preference.

@kriegaex kriegaex force-pushed the xalan-java-mvn-refactored branch from 816d616 to 010d8c3 Compare November 14, 2023 04:38
@kriegaex kriegaex changed the title Reformat Maven POMs, no functional changes Reformat Maven POM and assembly XLM, no functional changes Nov 14, 2023
@kriegaex kriegaex changed the title Reformat Maven POM and assembly XLM, no functional changes Reformat Maven POM and assembly XML, no functional changes Nov 14, 2023
@kriegaex
Reformat Maven POM and assembly XML, no functional changes
5d04033 
- Get rid of mixed tabs and spaces, use 2 spaces for indentation
  uniformly instead.
- Use line width 120 with very few exceptions.
- Use a uniform way to format multi-line XML comments, putting opening
  and closing tag parts on separate lines, indenting the text content by
  2 spaces.
@kriegaex kriegaex force-pushed the xalan-java-mvn-refactored branch from 010d8c3 to 5d04033 Compare November 14, 2023 04:39
@kubycsolutions
Copy link
Contributor

kubycsolutions commented Nov 14, 2023 via email

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 14, 2023 •
edited
Loading

Sorry, I thought I saw longer lines in other places, too. BTW, at the age of 52 years I am very much aware of the historic roots of the 80 character limit, having started programming on an 8 bit computer with text modes featuring a 40 character default with a "special mode" of 80 that needed to be started manually after each reboot.

But I am also aware of the fact that next to no developer, not even old guys like us, work with 80 character terminals anymore. Even in vi or Nano, usually I work with way more characters per line, unless I need to work in a Linux root terminal. But there, I do not write programs but maybe edit some config file, if my Linux kernel refuses to boot. Even then, I can scroll horizontally. I think, us older semesters should not impose an 80 character limit on younger developers just because of the good old days. Old developers are still developers, and in this profession (for me just a hobby since I turned agile coach almost 20 years ago) the only constant is change.

120 characters is widely accepted in many OSS projects and still narrow enough for me to fit more characters and a few tool windows on screen in my IDE. Besides, I was not reformatting the Java code, just a handful of POMs.

@jkesselm
Copy link
Contributor

jkesselm commented Nov 14, 2023

I've applied most of your changes manually, since we have agreed that we disagree on a few details of formatting style and since some obsolete comments could be ripped out entirely rather than just reflowed/re-indented. If that satisfies your intent, we can close this PR. If not, please merge and reconcile so it's focused on the remaining differences.

Thanks!

@jkesselm
Copy link
Contributor

jkesselm commented Nov 14, 2023

BTW, I've confirmed the problem in distribution/pom.xml. If I change the <package> type to pom, it overwrites itself with some form of binary. If I leave it as jar, it runs fine and the file is unharmed. So I have a workaround, but no idea why it would misbehave.

@kriegaex kriegaex closed this Nov 15, 2023
@kriegaex kriegaex deleted the xalan-java-mvn-refactored branch November 15, 2023 00:36
@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

Thanks, the uniform indentation is much better now. If you like vertical scrolling so much, keep your 80 character limit. I can live with it.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

BTW, I've confirmed the problem in distribution/pom.xml. If I change the <package> type to pom, it overwrites itself with some form of binary. If I leave it as jar, it runs fine and the file is unharmed. So I have a workaround, but no idea why it would misbehave.

I cannot reproduce it. You should get to the bottom of this irregular behaviour. I tried mvn clean verify on Maven 3.9.2. Please explain how to exactly reproduce this.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

I have a hunch: Remember when I said that you should not define plugins globally in the parent POM, at least not ones you only need in some modules? I see that you have quite a list of stuff going on there. For the source assemblies, you can get away with it, using the special runOnlyAtExecutionRoot parameter. But that kind of special parameter is e.g. unavailable for Maven Shade, which is also defined in the parent POM and therefore executes for each module, each tinme saying "Replacing original artifact with shaded artifact". the original artifact for pom artifact types probably is the pom.xml file itself, which is why it might get overwritten in your case, even though not in mine. Assembly for the source archive also runs for each module, albeit with a "Skipping the assembly in this project because it's not the Execution Root" message.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023 •
edited
Loading

As far as I can see, the only module in which Shade makes sense is the xalan module anyway, and there you redundantly defined the plugin already. It also is in the samples and serializer modules, where IMO it adds no value. Can you please explain why you think you want to shade each artifact? Is it even necessary for the xalan module, if you build a binary assembly later anyway? Usually, Shade or Assembly are only used, if you want to build a stand-alone application that should include all of its dependencies, to be able to run it from the CLI with an easy Java command.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

I think, you should also build the source distro in the distribution module, just like the binary one.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

Oh, I see what you are doing, using a centrally provided Apache plugin dependency for Assembly Plugin to be used on top level. Well, I kind of understand that this is easier than to custom-build your own assembly desceriptor. OTOH, this leads to directories being included that do not belong into the source distro. For example, I made some local changes to your project's Shade Plugin config, copying the resulting source/binary assemblies into a _tmp subdirectory before and after the change to be able to diff them afterwards. The whole _tmp subdirectory with dozens of MB of binaries ended up in the source distro. I.e., you need to be quite careful what is in your local workspace when running the build.

@kubycsolutions
Copy link
Contributor

kubycsolutions commented Nov 15, 2023 via email

@kubycsolutions
Copy link
Contributor

kubycsolutions commented Nov 15, 2023 via email •
edited by jkesselm
Loading

@kubycsolutions
Copy link
Contributor

kubycsolutions commented Nov 15, 2023 via email

@kubycsolutions
Copy link
Contributor

kubycsolutions commented Nov 15, 2023 via email

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023 •
edited
Loading

@kubycsolutions, it would be better to actually comment on GitHub and quote the relevant parts here

image

rather than to respond via e-mail. When reading inline, I do not really see what you are quoting. The inline quotes that are expandable here are always full quotes including e-mail signature etc.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

The exact command is in the mvnbuild.sh/.bat.

OK, I always use mvn directly, either from the IDE or the console. No extra wrapper scripts.

I believe it's "mvn clean package"... Which, if I remember correctly, is a stage or two past verify.

No, the verify phase is after package and also executes post-package integration tests, if any. Only later I noticed that this project is completely devoid of any automated tests, so I also would not have had to use -DskipTests for faster builds in the first place. 😉

Speaking of faster builds: I think, it would make sense to define Maven profiles for faster vs. more complete builds. If you actually work on the code in the future, you do not always want to build Javadoc and other types of documentation, distro JARs etc. Usually, you just want to compile and run automated tests, if any. Currently, the build is quite slow because of all the optional steps.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 15, 2023

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.sun JDK 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+.

WDYT?

@jkesselm
Copy link
Contributor

jkesselm commented Nov 15, 2023 via email

@jkesselm
Copy link
Contributor

jkesselm commented Nov 15, 2023 •
edited
Loading

Currently, xalan-test is a separate package. The idea was that it might be used/adapted as a test suite for other XSLT processors, who might not want to download all of Xalan to get it. Integration/regression testing requires downloading both and invoking the test drivers. It was also used for xalan-c, which I believe has been moved to the Attic as unused and extremely outdated.

I'm actually leaning toward pulling some or all of the test framework back into the Xalan-J. But that's a different work item.

Accepted as [XALANJ-2707] (https://issues.apache.org/jira/projects/XALANJ/issues/XALANJ-2707)

Re Maven profiles: Fine idea, but can be added later; I'd put it on the Jira backlog. On my fairly old machine running in Fedora/WSL, a full clean-and-build takes about 2 minutes (after Maven starts, which in this environment takes about 10 seconds). Incremental build without clean comes in at around half a minute. Not exactly instantaneous, but given that nobody but developers should be running this build at all frequently I don't consider it unreasonable. In an IDE most recompiles should happen incrementally upon file save rather than through Maven, so I don't think they're affected by this. Beware premature optimization of things not in the innermost loop.

Maven profiles for faster rebuilds, [XALANJ-2708] (https://issues.apache.org/jira/browse/XALANJ-2708)

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 16, 2023

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.

Then we need two variants of the Taglet classes, one using the old API and one the new one. The APIs need to be used in different ways, and the old classes are no longer available and therefore do not even compile on newer JDKs. The other way around, the new APIs are unavailable in JDK 8. I.e., duplicate taglet classes and conditional compilation using auto-activated, JDK-dependent profiles.

For now, that rules out requiring 9+. But we'll move forward someday. And users may want to build their own copies on newer versions.

See above.

It's orthogonal to the Maven migration, though.

That is true on the one hand, but OTOH maybe it is worth doing now. I have never used taglets at all, but it would be a nice puzzle to solve to adjust the taglet classes to the new API and to get the build running as described above. Next time I am stuck in another project and need an exercise to stretch my mind, I might look into it. I have actually begun to do so already, albeit superficially.

So I think the right thing to do is open it as a Jira Issue and get back to it.

https://issues.apache.org/jira/browse/XALANJ-2709

Fourth option: make the taglet itself introspect and pick the right set of APIs.

That is not a good option, because without reflection it would not work. See above.

@jkesselm
Copy link
Contributor

jkesselm commented Nov 16, 2023 •
edited
Loading

That is not a good option, because without reflection it would not work.

Who said without reflection? I'm not dogmatic about that; certainly it's a nit compared to BCEL.

And this is a pretty darned simple taglet.

But, yeah, switching at the Maven level is reasonable, arguably more so. I was just tossing reflection onto the table for completeness.

@kriegaex kriegaex mentioned this pull request Nov 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kriegaex @kubycsolutions @jkesselm