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

Index remote sources correctly. #5359

Merged
merged 1 commit into from Jan 22, 2018

Conversation

Projects
None yet
2 participants
@benjyw
Copy link
Contributor

benjyw commented Jan 19, 2018

Previously, if indexing only target roots, we didn't act on
the synthetic targets that wrap remote sources. This change
ensures that we follow the sources_target property in this case.

@benjyw benjyw requested a review from stuhood Jan 19, 2018

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jan 19, 2018

Do you have an example where the wrong thing happened? AFAIK, RemoteSources are now implemented just like codegen, where a synthetic (but otherwise real) language target is created to own the sources.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 20, 2018

Ugh, well, clearly I had an example, or I would never have thought of this... ;-)

But what I thought was going on was not what was going on. A remote_sources target points to an unpacked_jars target via its sources_target property. The behavior I was seeing led me to the belief that the synthetic language target is derived from the latter. But it turns out that this is false, and the actual problem was a stupid bug, which I will modify this change to be the fix for.

@benjyw benjyw force-pushed the benjyw:index_remote_sources branch from c653439 to f80b5cc Jan 20, 2018

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 20, 2018

Done. Sorry for the confusion.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jan 22, 2018

Ping?

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jan 22, 2018

Sorry, read the update on my phone, and thought that the result was that the PR would be closed. Looks good.

@benjyw benjyw merged commit 43b445a into pantsbuild:master Jan 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:index_remote_sources branch Jan 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment