Support fetching sources and javadoc in export using coursier #5254
Conversation
wisechengyi
changed the title
| @@ -54,7 +49,7 @@ def subsystem_dependencies(cls): | |||
| @classmethod | |||
| def register_options(cls, register): | |||
| super(CoursierMixin, cls).register_options(register) | |||
| register('--allow-global-excludes', type=bool, advanced=False, fingerprint=True, default=False, | |||
| register('--allow-global-excludes', type=bool, advanced=False, fingerprint=True, default=True, | |||
There was a problem hiding this comment.
This is analogous to --soft-excludes which was default to False, which is the opposite of this, so correcting it here. The change does not cross release boundary, hence skipping deprecation step.
| avro_lib_info = json_data.get('libraries').get('org.apache.avro:avro:1.7.7') | ||
| self.assertIsNotNone(avro_lib_info) | ||
| self.assertEquals( | ||
| self.assertIn( | ||
| 'avro-1.7.7.jar', |
There was a problem hiding this comment.
coursier and ivy have different paths, but what we care about is just the jar.
| def test_export_jar_path_with_excludes(self): | ||
| with self.temporary_workdir() as workdir: | ||
| test_target = 'testprojects/src/java/org/pantsbuild/testproject/exclude:foo' | ||
| json_data = self.run_export(test_target, workdir, load_libs=True) | ||
| print(json_data) |
There was a problem hiding this comment.
thanks. corrected.
| @@ -91,7 +86,7 @@ def _compute_jars_to_resolve_and_pin(raw_jars, artifact_set, manager): | |||
|
|
|||
| return jar_list, untouched_pinned_artifact | |||
|
|
|||
| def resolve(self, targets, compile_classpath): | |||
| def resolve(self, targets, compile_classpath, sources, javadoc): | |||
There was a problem hiding this comment.
How would you feel about making the new params optional with default values of False?
There was a problem hiding this comment.
Good question. I thought about that and the settled with this idea:
I think generally it is a good practice especially when the call stack is relatively simple. However in cases where the same args get passed further down into other functions, things may look confusing, so making it non-optional would make it clear during debugging where the args came from, and also more clear for readers that just want to skim the code.
For example:
def resolve(..., sources=false, javadoc=false)
x(sources, javadoc)
def x(sources,javadoc)
y(sources,javadoc)
def y(sources, javadoc):
// do other things
basically as a reader, it is hard to tell if sources, javadoc in y actually comes from the default value in resolve or the caller of resolve unless going back on the stack and then check how resolve is getting called, whereas non-optional args would make it clear that sources, javadoc in y is the exactly how resolve is getting called.
There was a problem hiding this comment.
sounds reasonable to me.
There was a problem hiding this comment.
The other approach that resolve methods take is to ask for a list of confs. Also, it looks like we don't use confs on the read-side of classpath products--and I'd generally like to move away from ivy confs as a pants concept.
| """ | ||
| Calling coursier and return the result per invocation. | ||
|
|
||
| If coursier was called once for classifier '' and once for classifier 'tests', then the return value | ||
| would be: {'': <first couriser output>, 'tests': <second coursier output>} | ||
| would be: {'default': [<first couriser output>, <second coursier output>]} |
There was a problem hiding this comment.
nit: s/couriser/coursier/
There was a problem hiding this comment.
haha thanks. the curse of french
Description
Earlier the experimental coursier task does not support fetching sources and javadoc, so this change adds such functionality.
Testing
Leveraging the existing tests with
@ensure_resolver