-
Notifications
You must be signed in to change notification settings - Fork 47
Migrate to Maven-based build #105
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
Conversation
Restructures serializer, xalan, and samples as sub-modules with dependencies on each other. Avoids carrying binaries in the project; fetch from Maven Central, or (in the case of the Javadoc taglet) carry the source in our own project. As part of that, eliminates dependencies on outdated versions of some of these, specifically java_cup and jlex. JFlex has replaced JLex, which permitted/required handling the look-ahead cases in the grammer rather than by reading ahead via code. Reworks "site" (documentation) generation. Some files may be landing in diffrent locations in the resulting directory tree; if that's an issue, it can be fine-tuned. Some javadoc errors were (sloppily) fixed before discovering that I can turn off "doclint" and let them continue to be warnings rather than build errors; these should be fixed properly at some point (in many cases by switching to using javadoc inheritance). Some files were converted from HTML to XHTML, since Maven's doc plugin knows how to handle the latter. JavaCupRedirect is no longer needed. On Windows, javadoc needed javax.rmi as an explicit dependency. CAVEAT: Some things wind up in different places than they did in the Ant build. If that bothers folks we can do more work to resove it, but I think it's mostly harmless. TODO: We still have some StyleBook files as well, mostly for the design documentation. I'm handling those with a "stylebook_docgen" script for now, invoked from the mvnbuild script/batch file; that should be moved into the pom's logic. NOT YET DONE: More of the non-Java files may want to be moved into resource subdirectories.
Note you can do this just when you're about to merge by choosing "Squash and merge" from the "Merge" web UI button options (there's a drop-down arrow on its right). |
|
I use that option all the time, works like a champ. |
It would be better to have separate commits for "rename files" and "modify files" changes. Currently, "Cutover from Ant-based build to Maven-based build" commit hides a lot, and it is hard to tell if it performs only the intended changes as there are a lot of renamed and modified files at the same time.
It looks like @jkesselm , could you please split "rename files" from "modify files" changes, exclude generated code from the PR and exclude duplicated files? |
|
Remove generated files: Sure. That snuck back in during the reset after recovering history. Duplicate: Ditto, I think; lemme check. There are currently multiple documentation paths, which may have done redundance between them. In general, though, Maven prefers xhtml. Refactor: Sorry, not undertaking that level of additional effort at this time unless the PR is incomprehensible without it. If I get multiple requests, or if you can provide an easy and reliable way to do it, I will reconsider. |
It is very hard to review in both GitHub UI (it is very cluttered), and the desktop tools (e.g. gitk, IDEA git, and so on). Many tools attempt to display full diff for the commit, which makes the diff almost impossible to follow.
It should not take more than 15 minutes, the outcome would be byte-by-byte the same, and the change will split into two commits. |
…ep them out (and comment it to explain this.
…ep them out (and comment it to explain this.
|
Removed generated files. Removed html files that had been refactored/edited into being xhtml files under src/site/xhtml. |
Haven't used git gui before. Not sure how to filter for "only renames". Are you proposing that be done manually? (It's also throwing error pop-ups when run under WSL, which may or may not be a problem.) (The force-pushes mentioned below were abortive attempts to do a Squash which I could then run this filtering against. Each of them reported conflicts, so I backed 'em out. I can fly git; I don't claim to be an expert.) |
d1f4a2b to
46d66d6
Compare
Yes. |
Could you please explain/provide a link to "maven prefers xhtml"? As far as I can remember, XHTML was abandoned long ago. What do you think of keeping the document as a regular HTML rather than converting it? |
Manual treatment is reasonable. Everybody will likely spend much more time trying to automate it. There's an alternative option:
The outcome will be the same as you have now, and "step 3" would produce a commit that includes much less of renames since most (all?) of the renames should be treated at "step 2". In any case, I suggest merging my PRs before proceeding with Maven. |
Aside: HTML5 still defines XML syntax for documents. |
|
And I've just reconfirmed that the output generated is .html files, not named .xhtml. So this is a difference that makes a difference to the build but no difference to the users. Working as designed and implemented. Valid question, but the answer is that it stays xhtml. |
So far you are the only person who has expressed concern. I'm willing to do a quick sizing to determine whether I can do this at all efficiently. No promises, so folks may or may not want to wait before starting to review. |
What do you want to convey with that? It is not the first time you say “the only person who has expressed..”, and I am truly puzzled what does it mean in the context of xalan. My rough estimation is that there are only a few persons who voice their opinion at dev@xalan at all. I know I am the first to raise the concern in Xalan, however:
Of course I can wait, however, I do not believe that “Vladimir is the only who raised a concern” is a valid excuse for not implementing a significant improvement, especially, when there’s no-one else to agree or disagree. The mailing list is virtually dead, so I would not expect a lot of comments |
|
2. It is important to keep git history reasonable for those who
encounter a bug in their systems, so they could analyze the nature
of the changes
I'm not sure I agree that the history for this commit is "unreasonable" as it stands. The history of each individual file is still clear, I think. (It is; just checked.)
Of course I can wait, however, I do not believe that “Vladimir is the
only who raised a concern” is a valid excuse for not implementing a
significant improvement
Agreed, but "X has raised a concern" isn't always sufficient for accepting it as significant (to anyone else) either.
There are others active here besides you and I. I don't think it's unreasonable for me to solicit their opinions. And lacking their input, see above. We agree that, currently, we mostly disagree on whether it's worth the additional effort. I'm willing to reconsider, hence the agreement to size the task. You've said you can do it in 15 minutes; my guess for myself is an order of magnitude higher.
|
Do you mean there are 10+ active contributors?
The effort is trivial, and it is you who selected to work on this PR while I have always suggested help with a migration to Gradle. It is unfair to reject ~15min efforts based on “it is not worth the additional effort” comments |
|
15min for you, perhaps. Longer for me. You could have volunteered those 15 minutes, y'know, rather than waiting for me to figure this out. But OK. I'll make an attempt. No guarantee of correctness is expressed or implied. ... Hour-plus later: After multiple attempts, haven't got it yet.
Shelving it for now while I think about what else I can attempt. If you have specific advice, that would be useful. |
|
Trying another approach. Which is taking hours, not minutes, since it's requiring that I take the list of added/deleted/moved/moved-and-changed files, construct scripts to confirm which added/deleted are really moved/copied and where things were moved from/to, and then turn those confirmation scripts into appropriate git operations. @vlsi : You say it can be done in 15 minutes. Please show me, specifically, how; I'd really like to learn whatever trick you're using. Otherwise, I'm on the verge of saying "nope, can't do it with reasonable effort, if I lose a reviewer that's acceptable." This can wait for Gary to have time to look at it. |
|
@jkesselm , I have suggested the step-by-step guide a couple of days ago in #105 (comment) Here's a 13min video of me following the exact steps: https://youtu.be/xnkfUGWFUWQ The renames I ended up with were slightly different from your current ones. There were several Other than that, I believe, it confirms that 15 minutes was a reasonable estimate for splitting the renames. |
|
As I said, the Maven site plug-in supports xhtml, not html, according to its documentation. Otherwise, I agree, I wouldn't have changed this. And since the work is already done, there's no reason NOT to stick with xhtml. We agree that we disagree on this one.
Location of resources/: valid point. Need to check that output is still consistent, and tweak appropriately if not, but I agree that's an improvement.
Thanks for the illustrated guide. I just wasn't seeing the path to get there, even with the description. Let's see if I can replicate this. You're using some Git syntax that I wasn't familiar with and will have to read up on.
(If not, we could check in your version as another branch, diff/test to confirm, then issue the PR from there; history on the individual file changes would be the same but you'd be credited for the moves, which would be fine. But lemme make the attempt to sort mine first.)
(BTW, pure nitpick, but Youtube is tending to bury the command line under the playback controls at the bottom of the screen, and I need to go fullscreen to read that text. Not going to ask that you redo it; this is for future reference if making a video for someone else. )
|
|
Uhm. If I start this process from a directory on branch maven-build, I fail when I get to I appreciate the opportunity to learn new git features, but I'm not sure what I'm doing wrong here. |
It says you should have put origin/master |
Video said otherwise, but -- Yes, running it with origin/master instead does appear to have produced a renames file. I really have to remember that trick. |
|
Did it by using the "renamed" file (VERY useful trick, thanks!) to drive a batchfile rather than "by hand" -- we each have our own ways to manage RSI -- but there should now be a branch called xalan-java-mvn-refactored which separates moving the files from editing, adding, and deleting, making them two different commits. (@vlsi: Just realized that In the "delete and replace" stage I probably lost moving the resources directory as you suggested. I'll do another commit. Sorry about that.) Outside of that one deliberate change, it should be identical to the maven-build branch. And it should have preserved all the history. I think. @vlsi, please examine. Does that satisfy, or are there further quibbles? Testing is in progress. But since the final stage was "copy contents from a working version", it should be OK. If so, I will probably close this PR and open a new one. Known glitch: Somehow the .sh scripts got permissions set to 755. I think we want them as 744; if the user wants to open them up further they can do so, but unnecessary execute permissions are asking for trouble. |
Why split the history/discussion across several PRs?
If you feel the scripts are unsafe, just remove them from the repository. Windows does not ask for execute bits, so you make the life of Linux/macOS users harder for no reason as you remove the execute bits. I guess you need to settle on
I've no idea how you test, however, I hope you'll add CI sooner rather than later. I guess you should not put
Would you scan through the second commit to identify excessive changes like xalan/tools/xalan2jdoc.jar, serializer/tools/xalan2jdoc.jar, and so on? Try something |
|
CI is not part of this PR. It has its own. Discuss it there.
Taglet jarfile: Agree it can be pulled out. Nitpick accepted; I'll commit that change.
The doclet jarfile stays for now. It is being kept as a reminder that we need to sanity-check whether we should be using it; if so we should do a source reconstruction for that too.
The XSLT samples need to be rewritten to not depend on the Sun zipfile included in that module. Until that happens, it needs to stay, since I have found no other source for it. (I haven't checked whether those samples actually still run, decades after their submission; they should be updated, and probably want test automation written.)
The scripts are safe as they stand. But unless there is Darned Good Reason to share them, scripts should never be executable for anyone other than the file's owner. Basic Unix security practice of minimum necessary permissions in order to reduce surface area, though I don't know whether that lesson was carried over to the Mac world.
|
|
Major reason I'm considering switching to a new PR is a combination of shedding the arguments about things other than the changes, and distrust of my Git skills in terms of forcing one branch over another. Closing this PR as "OK, we learned from it" and opening a new one is a fairly painless way to resolve both, and is pretty darned common Git practice in my experience. Main reason I haven't yet done so is that I haven't finished testing to convince myself that yea, verily, this is a consistent copy of the migration. |
|
Confirmed that the
(Yes, "propery". That should be fixed at some point.) OK, time to diff the two trees and see what else has gone AWOL. ADDENDUM: This appears to be the result of moving the serializer resource files. Moving them back until we can resolve what needs to be changed to support moving them. ADDENDUM: Confirmed. Committing reversion to the xalan-java-mvn-refactored branch. |
|
With that reversion (sigh, that's what I get for late changes), the refactored changeset now passes. Of course that's now refactoring plus about six more commits. I am NOT going to refactor again; they're small enough (especially given all the other changes) that folks can Deal With Them. I'm still uncomfortable overwriting this branch with that one, and would rather issue a new PR. |
Had to do some branch juggling in order to apply my changes on top of the history-recovery changes, but I think this includes everything from both.
Don't assume I haven't messed it up. Please retest, and tell me if any problems are detected.
Yes, I know I probably still have to squash this changeset.