Skip to content

Conversation

@kriegaex
Copy link
Contributor

@kubycsolutions Because you closed the previous PR, I am creating another one.

kriegaex and others added 8 commits November 15, 2023 07:34
Shade no longer runs on top level, but only in module 'xalan' where it
is required. Modules 'xalan', 'serializer', 'samples' are now
dependency-managed.
Actually, I am not sure if the Xalan maintainer really wants to exclude
it or have it in the distro as a transitive dependency for the 'samples'
module. Because it was excluded before my changes in this PR, I am
excluding it here, too, to avoid breaking changes. Another way to
achieve this would be to declare the dependency as 'provided' in the
'samples', but this over-use of 'provided' in this project seems odd to
me.
The old version 4.11 has security issues. Actually, the dependency could
be removed altogether, because there do not seem to be *any* automated
tests in this project.
- Fix typo (missing '$' in '${project.version}') I forgot to commit
  before.
- Use 'excludePackageNames=xalan2jtaglet' to avoid hen vs. egg problem,
  having to build and install the 'xalan2jtaglet' module before building
  a site. That is ugly and unnecessary sacred knowledge for new
  developers cloning the repo and building for the first time.
Again, there were tabs mixed with spaces and unclean indentation.
Replace '${project.build.directory}' - i.e. 'target' - by
'${project.basedir}', because we are not looking for sources in the
build directory. Now, the output resembles a source distro.

Actually, I have no idea why the original Apache source assembly
descriptor looks like that, at first glance it does not seem to make a
ton of sense.
@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 16, 2023

Continuing the discussion from #122, I do wonder if this type of binary and source distros are even still necessary nowadays. Just make sure to publish the right artifacts to Maven Central and instruct users to fetch whatever binary, javadoc and source artifacts you deploy to there. That should cover everything. I never work with zip or tar archives like that, if avoidable. I always try to fetch everything I need via Maven. The only possible exception are installers for applications containing multiple artifacts, if I want to just unzip or install them and run the CLI application without setting up a Maven project. But I really cannot remember many such cases. I think, those manually built distros are simply an anachronism in most cases. Maven has all the tooling to automatically build binary, source and javadoc JARs. Central requires all of them for deployment nowadays, anyway.

Update: I have an exception from another OSS project I am maintaining: AspectJ, while publishing several artifacts alongwith source and javadoc artifacts on Maven Central, also builds one more artifact: an installer. It is an executable JAR which sports a minimal GUI that installs the main AspectJ artifacts into a user-defined directory and sets up a few Windows/UNIX batch files to run the AspectJ compiler and aspect-enabled javadoc generator from CLI. This is for users who want to compile Java and AspectJ files from the console without Ant, Maven or Gradle. The installer used to be published on the AspectJ project website. Nowadays, it gets attached to the GitHub release. We do not push it to Maven Central, because it is not a library but in a sense a collection of CLI applications with different entry points. I have, however, no idea how many users still use AspectJ without any build tools. So in a way, it is also an anachronism.

@kriegaex kriegaex changed the title Improve shade assembly, part 2 Improve Maven Shade and Assembly usage, part 2 Nov 16, 2023
@jkesselm
Copy link
Contributor

I have the same question about necessity... But since I have no answers, I'm not going to shut that down yet.

First migrate. Then optimize. There's already more change in this than I had hoped.

@kriegaex
Copy link
Contributor Author

First migrate. Then optimize.

Well, we can leave the camp ground behind a little cleaner than we found it - boyscout rule. But as you can see in this PR, I am retaining the assemblies, merely fixing things. Not even with a clean build would it work with the new source assembly, the way you set it up. It would always include build artifacts from your non-standard build directory. Really, there is target for that, an extra directory really is not necessary. It just makes the build more complicated and slower. You are catering to Ant precedence here, which is why I had to fix something that would not have needed fixing if you would have stuck to the Maven way. You would still have been able to create your assemblies that way.

There's already more change in this than I had hoped.

Do you mean in the migration branch in general or specifically in this PR? I was hoping you could merge it soon, because I solved the "taglet on JDK 9+" problem locally and have that feature branch based on this one.

@jkesselm
Copy link
Contributor

The build directory isn't target. It's the point at which the behavior of this Maven should result in the same artifacts as the old Ant. As such it is both proof of success, and the point at which other builds -- specifically the test suite -- interact with this one when run in the same directory space.

Technically I should indeed be producing the assemblies in target and then copying them to build as an afterthought. I have found copying to be overly complicated, though, which has inclined me toward writing the assemblies directly to build for now. Making the copying yet another dependent module might work, but ugh... that's staring to become uglier than make files (though admittedly more portable, which combined with the repository is what's kept me trying to wrap my head around Maven.)

build/ will become less necessary after we have gotten past Maven-build acceptance and started reworking the tests into a Maven-nativeish form. But for now, we need it if we want people to be convinced we haven't broken anything.

My assembly should be excluding build, exactly as it excluded target. If it isn't, I'll fix that. 

There's already more change in this than I had hoped.

Do you mean in the migration branch in general or specifically in this PR?

Migration in general. And yes, I too am hoping we can merge soon. Getting feedback from you about how Maven wants to think about things has been hugely helpful; if I can just get the distribution zips to the point where they're equivalent to the Ant versions (modulo deliberate restructuring into modules and some dependency updates), and rerun regression tests on the jars with version number stripped off them, I'm just about ready to pull the trigger. I would prefer to walk through the maven build with Gary G. before cutting over, to get one more pair of eyes on it, but I wanted to get it at least metastable before trying to get on his calendar.

@jkesselm
Copy link
Contributor

uglier than make files

It should be noted that makefiles, done properly, are also declarative. They don't have as good encapsulation around "plug-ins", or as many canned solutions available off the shelf, and they aren't platform-independent. But if anything they are more dependency driven, since they rely solely on artifact dependency rather than phase or module dependency.

In fact the use of the latter. combined with the inconsistencies from plugin to plugin over things that could be core behaviors, suggests that Maven is working but not yet achieving its stated philosophy. Which is OK, as long as folks recognize that it may getting toward time to gather the lessons learned and consider breaking changes to bring everything back in line -- another major-version release.

@kriegaex
Copy link
Contributor Author

I am now done with making the build fit for JDK 9+, tested up to JDK 21. If you could please review and merge this PR soon, then it would not get so messy to merge the next one, because it is dependent on this one. I do not want either of us to end up in merge hell later.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 16, 2023

As for making the source and binary assemblies more closely resemble that 2.7.1 ones, I can help with that. But then, I need more information about what exactly ought to be the same and what you indend to change or restructure. That discussion would be way too tedious and non-interactive to have on GitHub, though. I suggest to connect on Telegram, Gitter or WhatsApp in order to increase information bandwidth and speed up feedback cycles.

You find my contact data in my Stack Overflow profile, click "Read more" and scroll down.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 16, 2023

It should be noted that makefiles, done properly, are also declarative.

I know, even though it has been ages since I used one.

if anything they are more dependency driven

If done right, they are actually great (and fast as hell) for incremental builds in which really only the absolute minimum of what has changed will be rebuilt. I remember, in the old times I managed to speed up a cross-compile build from Linux to an embedded platform from 1 hour for tiny changes to a few seconds, just because I took the trouble to stringently define dependencies and not build everything or at least way too much "just to be safe".

Maven is less fine-granular in this regard and therefore more "wasteful". In exchange, you get productivity in other areas when using Maven. But still, if done right (as always), you can make Maven builds faster and not always build everything. Hence my idea with profiles. You might think that 2 minutes are fast. I prefer 2 seconds, especially if I rebuild 50 times in the process of optimising some piece of code or Maven POMs, which lately I have done in Xalan-J to help you a bit. I lose more time waiting for builds than I need to make changes in POMs.

@kubycsolutions
Copy link
Contributor

kubycsolutions commented Nov 16, 2023 via email

@jkesselm
Copy link
Contributor

Sanity-checking local merge. Fast-forward was happy.

Question while I've got you: We have a few files that need to be filtered to insert version info, currently saved as whatever.src. The Ant build filtered them into whatever.java. Looking at Maven filtering, I don't see a filter-and-copy operation that would work for these. Am I missing something, or do I have to check the raw version in as .java so they get filtered properly, or...?

@jkesselm jkesselm merged commit 4af37e7 into apache:xalan-java-mvn-refactored Nov 16, 2023
@jkesselm
Copy link
Contributor

Smoketests report no regressions. (A few known failures are not counted in the overall result; technical debt, there should be work items open for them.)

Good enough for classical. Merging, with some some responses to the open questions in the merge comment.

Thanks again! We aren't quite over the hill yet, but I think I can see the summit from here. (Of course that describes me too.)

@kriegaex
Copy link
Contributor Author

We have a few files that need to be filtered to insert version info, currently saved as whatever.src. The Ant build filtered them into whatever.java. Looking at Maven filtering, I don't see a filter-and-copy operation that would work for these. Am I missing something, or do I have to check the raw version in as .java so they get filtered properly, or...?

Let us deal with that after merging the XALANJ-2709 PR. Some basic thoughts:

  • Why make matters complicated by using a different file extension? Just name the resource file like the final file and let a filtering plugin do the rest.
  • For filtering source code, there is Templating Maven Plugin. I never used it before, but the basic idea is that it both filters and copies the file, but usually from src/main/java-templates to target/generated-sources/java-templates. Then the latter is added as a source folder. That would work just fine in a Maven build, but how well it would be understood by an IDE is to be tested.
  • I would use a completely different approach, which is much simpler and avoids having to commit your Java files, just because version numbers have to be baked into them: Let the Java files read those version numbers from a properties or other type of resource file. That is standardised and easy to handle with resource filtering.

@kriegaex kriegaex deleted the improve-shade-assembly branch November 24, 2023 03:44
jkesselm added a commit that referenced this pull request Jan 1, 2024
* Ignore IntelliJ *.iml project files

* Fix Maven Shade usage

Shade no longer runs on top level, but only in module 'xalan' where it
is required. Modules 'xalan', 'serializer', 'samples' are now
dependency-managed.

* Exclude jboss-rmi-api_1.0_spec from binary distribution

Actually, I am not sure if the Xalan maintainer really wants to exclude
it or have it in the distro as a transitive dependency for the 'samples'
module. Because it was excluded before my changes in this PR, I am
excluding it here, too, to avoid breaking changes. Another way to
achieve this would be to declare the dependency as 'provided' in the
'samples', but this over-use of 'provided' in this project seems odd to
me. 
    jkesselmn adds: `Provided` was a quick kluge; some library jars
    that weren't packaged into assemblys by the ant build were being
    copied by the mvn build until I added this. Needs to be rechecked,
    and there is probably a better way to express it in mvn.

* Bump JUnit to 4.13.2

The old version 4.11 has security issues. Actually, the dependency could
be removed altogether, because there do not seem to be *any* automated
tests in this project.
    jkesselmn adds: The intent is that there will be. I'd rather declare it now,
    rather than wait for first use.
 
* Fix and improve site generation

- Fix typo (missing '$' in '${project.version}') I forgot to commit
  before.
- Use 'excludePackageNames=xalan2jtaglet' to avoid hen vs. egg problem,
  having to build and install the 'xalan2jtaglet' module before building
  a site. That is ugly and unnecessary sacred knowledge for new
  developers cloning the repo and building for the first time.
    jkesselmn adds: Our javadoc does use that otherwise-missing taglet
    (which is why we decompiled it and are carrying it here as a stopgap).
    Site depends on the javadoc, so this jarfile does need to be present
    before then. I wasn't sure how best to disentangle it short of doing
    more inter-module dependencies; if this works, great!
 

* Remove commented-out plugins, fix whitespace issues

Again, there were tabs mixed with spaces and unclean indentation.
    jkesselmn add: Sorry. I need to get my XML Emacs configuration 
    switched to spaces-only indentation, rather than trying to remember
    to normalize spaces.


* Fix source assembly

Replace '${project.build.directory}' - i.e. 'target' - by
'${project.basedir}', because we are not looking for sources in the
build directory. Now, the output resembles a source distro.

Actually, I have no idea why the original Apache source assembly
descriptor looks like that, at first glance it does not seem to make a
ton of sense.

---------

Co-authored-by: Joe Kesselman <131899227+jkesselm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants