Skip to content

Conversation

@kriegaex
Copy link
Contributor

@kubycsolutions For more info, please read all commit comments, especially the ones about jboss-rmi-* and JUnit.

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.
@kriegaex
Copy link
Contributor Author

Joseph, please also note that if your main goal is compatibility with previous Xalan-J versions with regard to the way the source and binary distribution archives look like, there are quite significant changes. Maybe those are intended and you want to restructure the distros a bit. I briefly took a peek into the Xalan-J 2.7.1 binary distro, and it looks quite different indeed. However, this PR mainly wants to streamline and fix the way you use Maven Shade.

@jkesselm
Copy link
Contributor

Thanks for the shade work.

The code had to be restructured somewhat to divide it into Xalan modules, and a few other tweaks were needed to get it running against Maven-provided resources. So some changes in the zip are acceptable as long as it includes all the equivalents and the jarfiles have expected names. I'm fine-tuning that now; the work on src has moved me back a bit.

QUESTION: How would I rename the parent module? If I try renaming xalan-project to *xalan-j in all the artifactIds, I get complaints from maven that some of the child modules were expecting the old artifactID. I presume this is something to do with local caching, but I'm not sure how to reliably flush that cache.

@jkesselm jkesselm merged commit ef32085 into apache:xalan-java-mvn-refactored Nov 15, 2023
@jkesselm
Copy link
Contributor

There will be junit dependencies, I hope, if we decide to integrate xalan-test into xalan-java. I'm in favor of leaving it as a declared dependency for now.

@jkesselm
Copy link
Contributor

Problem. After merging your change, I'm getting

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-site-plugin:3.7.1:site (default-site) on project xalan-project: Error generating maven-javadoc-plugin:3.6.0:aggregate report: Unable to resolve artifact:groupId = 'xalan'
[ERROR] artifactId = 'xalan2jtaglet'
[ERROR] version = '{project.version}'
[ERROR] classifier = 'null': Failure to find xalan:xalan2jtaglet:jar:{project.version} in https://repo.maven.apache.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced

When I try to add it to the dependencies, for sequencing, I'm told I have to specify version -- unlike the other dependencies in my project. That's odd...

There might a time window issue. The taglet needs to be packaged before javadoc is run. But site needs to run before package if I'm to include the javadocs in the distribution jarfiles. I wonder whether the provided flag was letting me get away with this in the past....

@jkesselm
Copy link
Contributor

[ERROR] version = '{project.version}'

Actually, that makes me nervous. Shouldn't there be a real version number there? Do I need an explicit dependency?

@jkesselm
Copy link
Contributor

Whatever it is, reverting the merge cures it. Want to investigate and try again?

@jkesselm
Copy link
Contributor

Possible that this was a collision between your work on shade and my having started to do something similar. I can try taking one change at a time and see if I can isolate the issue -- or confirm that it's merge madness.

@kriegaex
Copy link
Contributor Author

Problem. After merging your change, I'm getting

[ERROR] version = '{project.version}'

That was a simple typo, I forgot $. It must be ${project.version}, of course. I had not tested site generation before the push and fixed that locally immediately yesterday, but along with other local changes I forgot to stage, commit and push it. Will do.

@jkesselm
Copy link
Contributor

Ah. I see the issue, I think: top-level pom.xml in the changeset is indeed missing a $. Sigh.

@kriegaex
Copy link
Contributor Author

kriegaex commented Nov 16, 2023

I also want to push another improvement for site generation. The build config is unclean in one regard. Stay tuned.

@jkesselm
Copy link
Contributor

Confirmed that the missing $ was indeed the problem. Awaiting patch; I'll be interested to see what you change.

And thanks again for sanity-checking this. Learning Maven while trying to apply it to a nontrivial project is not optimal. Fools rush in, and all of that...

@jkesselm
Copy link
Contributor

Currently rolled back to before the merge.

@kriegaex
Copy link
Contributor Author

I just noticed you merged other changed into this branch. Got to rebase the commit onto those and run a local build first. I also quickly want to check what you changed, to avoid that any of it logically clashes with my changes.

@jkesselm
Copy link
Contributor

Quoting the Flying Karamazov Brothers: "Juggling is in everyone's job description."

@kriegaex
Copy link
Contributor Author

Your new source distro built in the distribution module does not look quite right. The source assembly config copied from the Apache one sucks everything into the assembly, including my .idea directory and build. I am going to add more excludes for now, but actually it would be safer to write a clean config which explicitly says what to include.

@jkesselm
Copy link
Contributor

The only way I know to build a really safe distro is a clean check-out. Otherwise you wind up either having insufficient excludes to handle every case, or so many includes that you wind up having to maintain it every time you add a file. Thermodynamics...

The source assembly I'm using is based directly on the "official" Apache standard assembly, changing only what I must. So we're no worse off than any other Apache project which uses that canned solution.

This is why I wanted to be able to tap .gitignore's contents from Maven plugins; it's already a declaration of what should not be committed, and gets maintained as developers discover the need to maintain it; having to maintain the source build separately is painful and should be avoidable. If folks want to also support other SCCS ignore lists, great, but given how dominant Git is these days...

I tend to feel that .** should be in the exclude list, perhaps with explicit exception for .gitignore or as manually managed for specific projects. Though .gitignore can be explicitly commited to keep it from ignoring itself.

@kriegaex
Copy link
Contributor Author

Please try not to change anything there while I am experimenting, to avoid clashes.

@kriegaex
Copy link
Contributor Author

The only way I know to build a really safe distro is a clean check-out.

I would consider that a smell.

Otherwise you wind up either having insufficient excludes to handle every case, or so many includes that you wind up having to maintain it every time you add a file.

I disagree. I do not think that so many includes would be necessary. Includes are safer in this case than excludes, IMO. OK, if you add new modules you can forget them. But it is due diligence to check if the assemblies contain new stuff. BTW, the same type of oversight can happen with excludes. Anyway, in the new PR #123, for now I tried to salvage the existing assembly descriptor with its excludes.

@jkesselm
Copy link
Contributor

Please try not to change anything there while I am experimenting, to avoid clashes.

Ok. Give me a time horizon?

Sorry; didn't realize there wasn't an easy "reopen" button. Opening a new PR's entirely reasonable.

@jkesselm
Copy link
Contributor

BTW, the same type of oversight can happen with excludes.

Quite aware of that; my point was that neither is a panacea, and as far as I can tell which has the advantage depends on what kinds of errors you think are most common. I suspect this, like the line length, is more style than substance. Not opposed to looking at the alternative solution, but I don't think it needs to be done right now. We're still in "make it work".

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.

2 participants