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

Move support for sources into Target. #4786

Closed
wants to merge 1 commit into from

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Aug 4, 2017

There were several target subclasses re-implementing the boilerplate and
Target otherwise had intimiate knowledge of sources and an expectation
that the field existed for auxially methods.

This also allows for a general sources bag that can be used to express
file dependencies with no other expectation about the files' type or
purpose.

@jsirois
Copy link
Contributor Author

jsirois commented Aug 4, 2017

This is part of the work for #4780

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

I think the reason this is the way it is currently is that "not all Target subclasses accept the sources arg"... so I think that this change was cause all targets on http://www.pantsbuild.org/build_dictionary.html to accept that arg... but many of them would not do anything.

Would it be possible to continue to have this be opt-in, but make the opt-in much cheaper?

@jsirois
Copy link
Contributor Author

jsirois commented Aug 4, 2017

Hmm, yeah. I was thinking there weren't many cases of this, but there are. I'll re-evaluate.

@jsirois jsirois force-pushed the jsirois/issues/4780 branch from d7fc28b to e236bab Compare August 4, 2017 19:49
There were several target subclasses re-implementing the boilerplate and
`Target` otherwise had intimiate knowledge of sources and an expectation
that the field existed for auxially methods.

This also allows for a general `sources` bag that can be used to express
file dependencies with no other expectation about the files' type or
purpose.
@jsirois jsirois force-pushed the jsirois/issues/4780 branch from e236bab to 78effed Compare August 8, 2017 02:56
@jsirois
Copy link
Contributor Author

jsirois commented Aug 8, 2017

I have a branch introducing a Files target that is ~Resources and which Resources can subclass to satisfy existing type tests that want to embed resource files in binaries. I'll move some of the other small changes here that are worthwhile to tiny PRs and close this out once the new Files PR is posted.

@jsirois
Copy link
Contributor Author

jsirois commented Aug 8, 2017

Miscellany broken off into cpp: #4793 and go: #4794
I'll close this now and get the Files PR out after #4792 lands.

@jsirois jsirois closed this Aug 8, 2017
@jsirois jsirois deleted the jsirois/issues/4780 branch August 8, 2017 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants