Skip to content
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

Minor java improvements #3761

Merged
merged 5 commits into from May 23, 2022
Merged

Minor java improvements #3761

merged 5 commits into from May 23, 2022

Conversation

djh82
Copy link
Contributor

@djh82 djh82 commented Aug 2, 2020 •
edited

Contributor Checklist:

  • I have created a new test or updated the unit tests to cover the new/changed functionality.
  • I have updated CHANGES.txt (and read the README.rst)
  • I have updated the appropriate documentation

Here are some minor Java improvements for discussion; I've yet to add tests and update the CHANGES.txt/relevant documentation, but would be happy to do so if there it is decided that these changes are worth integrating.

@bdbaddog
Copy link
Contributor

bdbaddog commented Aug 2, 2020

Before I dig around the code, please summarize the improvements (with an eye towards being able to review the PR)?
(rather than me having to figure out what the change is supposed to do?)

@bdbaddog bdbaddog added the Java Java tools and language support label Aug 2, 2020
@djh82
Copy link
Contributor Author

djh82 commented Aug 2, 2020 •
edited

This PR aggregates 3 of the changes we've made to improve java handling:

  1. Introduces a scanner which handles both JAVACLASSPATH and JAVABOOTCLASSPATH, otherwise we have to litter our code with Depends.
  2. Improves classpath generation, since if you added a nodelist to the JAVA*PATH env variable, you'd end up with a.jar:[b.jar]
  3. Tries to improve the handling of generated source files to prevent constant rebuilding. Currently scons assumes generated files will create class files in the default package, but 9 times out of 10 this is unlikely to be the case. The standard (but not required) is to put generated source files on their package directory, and hence this change tries to deduce that. Java protobuf is a good example. Note that this could still get it wrong, but it's no less wrong, and it's probably right more of the time.

@djh82
Copy link
Contributor Author

djh82 commented Sep 12, 2020

#3761 has a partial fix for this

@bdbaddog
Copy link
Contributor

bdbaddog commented Oct 9, 2020

@davidhart82 - can you add some tests to cover these changes?

@djh82 djh82 changed the base branch from master to aix-xlc_r-is-for-threads October 11, 2020 11:00
@djh82 djh82 changed the base branch from aix-xlc_r-is-for-threads to master October 11, 2020 11:00
@djh82 djh82 changed the base branch from master to aix-xlc_r-is-for-threads January 12, 2021 10:00
@djh82 djh82 changed the base branch from aix-xlc_r-is-for-threads to master January 12, 2021 10:00
@djh82
Copy link
Contributor Author

djh82 commented Jan 12, 2021

@bdbaddog I've scrubbed fix number 3; it might need more work. But I've added tests for the other 2 fixes.

@djh82 djh82 closed this Jan 12, 2021
@mwichmann
Copy link
Collaborator

Is there a replacement coming?

@djh82 djh82 reopened this Jan 12, 2021
djh82
djh82 commented Jan 12, 2021
View reviewed changes
SCons/Scanner/Java.py Outdated
for lib in classpath:
if SCons.Util.is_String(lib) and "*" in lib:
result += env.Glob(lib)
elif os.path.isdir(str(lib)):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to find all of the in-memory and on-disk nodes; is there a better way of doing this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SCons Glob should find both..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The age old scons glob isn't recursive problem - need to find classes in the current directory and below.

@djh82
Copy link
Contributor Author

djh82 commented Jan 12, 2021 •
edited

Is there a replacement coming?

@mwichmann I got fed up with it, and was going to bin it. But I had a change of heart, reduced scope a little, and now I think it works.

djh82
djh82 commented Jan 12, 2021
View reviewed changes
SCons/Scanner/Java.py

Class path entries that are neither directories nor archives (.zip or JAR files) nor the asterisk (*) wildcard character are ignored.
"""
classpath = env.get('JAVACLASSPATH', [])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might work for JAVABOOTCLASSPATH now

djh82
djh82 commented Jan 12, 2021
View reviewed changes
SCons/Scanner/JavaTests.py
JAVACLASSPATH=[test.workpath()])
s = SCons.Scanner.Java.JavaScanner()
deps = s(DummyNode('dummy'), env)
expected = ['Test.class', 'com/Test.class']
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if this should pick up jars too

@djh82 djh82 changed the title [WIP] Minor java improvements Minor java improvements Jul 5, 2021
@mwichmann
Copy link
Collaborator

This one has set open for a while now - what do we need to do to get it over the line? I'd probably change the title a little bit - adding a scanner feels like a little more than just "minor improvements". I pulled the PR to here and rebased it to head and the added tests still pass fine. I'm no Java person so not much further help from me.

A few nitpickery notes for updates:

  1. Run Black on the two added files - new files might as well enter the picture in the expected eventual style (I know it doesn't say this anywhere!)
  2. The CHANGES.txt snip needs to move to the next-release location, a rebase leaves it where it was (under 4.2.0)
  3. Seems like a new scanner should get a snip in RELEASE.txt
  4. Check for trailing whitespace

@djh82
Copy link
Contributor Author

djh82 commented Dec 20, 2021

@mwichmann I'd almost given up on it! We use this code everyday, so am happy to fix it up inline with your suggestions. Hopefully I'll get some time this week to do that.

@bdbaddog bdbaddog merged commit f709352 into SCons:master May 23, 2022
@mwichmann mwichmann added this to the 4.4 milestone May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdbaddog bdbaddog bdbaddog left review comments

Assignees
No one assigned
Labels
Java Java tools and language support
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@djh82 @bdbaddog @mwichmann