-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
interpreter: Move validation of BuildTarget(extra_files) to Interpreter #15189
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
base: master
Are you sure you want to change the base?
interpreter: Move validation of BuildTarget(extra_files) to Interpreter #15189
Conversation
a9c70d1 to
868c0cf
Compare
|
@bonzini are you happy with this version? |
|
Yes, sure. |
| continue | ||
| # TODO: this prevents built `File` objects from being used as | ||
| # extra_files. | ||
| trial = os.path.join(self.environment.get_source_dir(), i.subdir, i.fname) |
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.
Is this changing? Are we adding support for new things here?
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 is checking that we don't use generated files, which we could also check with File.is_built.
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 updated the commit message to have that.
I originally had several patches in this series that made the transformation one change at a time, but I feel like I always get asked to squash that, so I didn't send it as a split series.
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.
Thanks. I was mostly confused since I wasn't sure how to read the TODO. It seemed to be implying that we want to change the status eventually and start allowing generated files.
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 wrote the TODO, and originally I thought this was buggy behavior, but then realized this field is for files being grouped in an IDE, which I would think generally people don't want to edit generated files so this actually makes sense.
This gets us to the point that the build layer can assume it's getting valid inputs. We switch from using a check that files exist (expensive) to checking `File.is_built`, which achieves the same thing, but without doing filesystem I/O.
868c0cf to
576bbf2
Compare
This gets us to the point that the build layer can assume it's getting valid inputs.