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.

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

Add Depends() method. #17

Merged
merged 8 commits into from Nov 14, 2017
Merged

Add Depends() method. #17

merged 8 commits into from Nov 14, 2017

Conversation

dmoody256
Copy link
Contributor

@dmoody256 dmoody256 commented Oct 26, 2017

This issue was originally created at: 2001-07-19 22:00:00.
This issue was reported by: stevenknight.
stevenknight said at 2001-07-19 22:00:00

Tracking explicit dependencies will be easiest first.

issues@scons said at 2001-07-19 22:00:00

Converted from SourceForge task item 34792

stevenknight said at 2006-05-20 20:49:15

No white space in keyword.

…. This was taken from the similar Java method in the javac tool. Updated the Jar builder to have an unambiguos name.
… compiled and are in the resulting jar file. Also updated the swig dependency test to throw no result from what seems to be a non java related bug.
…he test, and considers no results a pass because some wont be able to be tested with travis at the moment (i.e windows).
# jar target should not be a list so assume they passed
# no target and want implicit target to be made and the arg
# was actaully the list of sources
if SCons.Util.is_List(target):
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I'm thinking a warning should be issued rather than quietly discarding the target specified by the user?

Copy link
Contributor Author

@dmoody256 dmoody256 Nov 13, 2017

Choose a reason for hiding this comment

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

I agree, warnings for when things are getting weird is good, I'll update with a commit on this request soon.

Also on this note, is there any expected behavior when you pass multiple targets to the Jar builder? I didn't see anything in the man page. In this case, I assumed this behavior from the Java multi-step.py test, where it passes only a list of sources to the Jar builder: https://github.com/SConsProject/scons/blob/a583f043aec89e58bf4ab0ab20cc039299eff1df/test/Java/multi-step.py#L377

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully expected behavior is covered in the docs and/or the test suite.

Probably worth putting some notice that you'll be dropping the target list in the release notes (src/CHANGES.txt)?

Do any of the tests hit this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to roll out 3.0.1 and would like to get this change in. Any chance you can do the above today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully expected behavior is covered in the docs and/or the test suite.

The man page for Jar says it will build "a" jar archive so I assume that implies a single target:
http://scons.org/doc/HTML/scons-man.html

Jar() , env.Jar()
Builds a Java archive (.jar) file from the specified list of sources.

Probably worth putting some notice that you'll be dropping the target list in the release notes (src/CHANGES.txt)?

I will update the CHANGES.txt about the target list not being used.

Do any of the tests hit this case?

No tests test multiple targets passed to Jar, I can write a test for that but I still am not sure what is expected in that case. Currently it assumes if your first argument is a list (base builder class first arg is target), then you meant implicit target and just passed the list of sources, so if someone was trying to pass a list of targets and a list of sources it would ignore the sources and assumes your targets were the sources. Maybe this case should be an Error and stop the build?

Any chance you can do the above today?

I can update regarding the warning message and the CHANGES.txt in a few hours.

try:
java_jar = env['BUILDERS']['Jar']
java_jar = env['BUILDERS']['JarFile']
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you change the name of the builder here from Jar to JarFile?

Copy link
Contributor Author

@dmoody256 dmoody256 Nov 13, 2017

Choose a reason for hiding this comment

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

Yes, I added a builder wrapper in its place with the AddMethod function:
b54f416#diff-06522ed2191b2caac07035c96d8cbfa4R199
I took this design from the Java builder which has a similar design pattern for taking in sources from both directories and java sources, see below for comparison:
https://github.com/SConsProject/scons/blob/a583f043aec89e58bf4ab0ab20cc039299eff1df/src/engine/SCons/Tool/__init__.py#L954
https://github.com/SConsProject/scons/blob/a583f043aec89e58bf4ab0ab20cc039299eff1df/src/engine/SCons/Tool/javac.py#L163-L167

@dmoody256
Copy link
Contributor Author

Updated and build passing here:
https://travis-ci.org/dmoody256/scons/builds/301730660

@@ -98,18 +98,30 @@ def Jar(env, target = None, source = [], *args, **kw):
# jar target should not be a list so assume they passed
# no target and want implicit target to be made and the arg
# was actaully the list of sources
if SCons.Util.is_List(target):
if SCons.Util.is_List(target) and source == []:
Copy link
Contributor

Choose a reason for hiding this comment

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

In general all values returned from builders are NodeLists. So it's possible a user could specify a target as a list, but that list could have only one item in it. Perhaps better to also check if the list has more than one item here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind. Your solution seems reasonable given a target list with one or more targets.

@bdbaddog bdbaddog merged commit 5d87f7f into SCons:master Nov 14, 2017
@bdbaddog
Copy link
Contributor

Looks like this broke someone's usage.
Can you take a look at:
https://pairlist4.pair.net/pipermail/scons-users/2017-November/006469.html

And respond on users list?

@dmoody256
Copy link
Contributor Author

@bdbaddog

Yeah, it's because they passed a list of lists when they made a list from the manifest and the list of class files. If they extend the class file list with the manifest instead it should work. However, I think the Jar builder should be able to handle this case.

I propose the Jar builder breaks down the lists of lists from the source arg into a single list and uses the single list in place? Thoughts?

Sent from my Galaxy Note5 using FastHub

@bdbaddog bdbaddog changed the title Fix Jar again and add Travis CI script Add Depends() method. Jan 2, 2018
@bdbaddog bdbaddog added the P3 label Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants