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
jarring up resources #1503
jarring up resources #1503
Conversation
@@ -25,6 +27,12 @@ def register_options(cls, register): | |||
super(PrepareResources, cls).register_options(register) | |||
register('--confs', advanced=True, type=Options.list, default=['default'], | |||
help='Prepare resources for these Ivy confs.') | |||
register('--resources-dir-patterns-for-jarring', advanced=True, type=Options.list, default=[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the default be the resource value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that option names are already prefixed with the task name. So the long form of this one would be:
--resources-prepare-resources-dir-patterns-for-jarring
...what I'm saying is: use shorter names (and particularly names that won't be redundant with the task name.)
""" | ||
:param sources: Files to "include". Paths are relative to the | ||
BUILD file's directory. | ||
:param use_jar: Whether or not the resources will be zipped into a jar. This flag is ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should make it clear that the flag doesn't affect bundle/binary (when things are always jar'd.)
@Test | ||
public void testResourceJar() throws Exception { | ||
assertEquals( | ||
"1234567890", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually confirm that there is a jar on the classpath... you'd have to do something like URL.getProtocol == 'jar'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought about that. the thing is because use-jar is by default disabled in OSS pants, if I assert jar here, it would be broken unless the tests is run with:
./pants tests testprojects/tests/java/org/pantsbuild/...../resourcesjar --resources-prepare-use-jar
this testproject addition is mostly for manual testing.
like i said, I haven't added any unittests or IT tests for the changes yet. So this pull request isn't complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the test search the actual classpath. How come that got dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from an earlier Stu's comment:
Rather than matching the jar name, should just test that a particular resource you have defined is located in a jar. The name isn't relevant.
…b/src/test/java/org/pantsbuild/duplicateres/examplea:exampleb
…es_with_jar Conflicts: build-support/bin/ci.sh
No description provided.