Skip to content

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Mar 5, 2018

This fixes the cause of an assert failure @davidungar observed building a project with bridging PCH and batch mode: we were passing all actions -- InputActions and otherwise -- from the merged jobs' input actions arrays to the JobContext's InputActions field. We need to pass along only the InputActions for that, and independently select and propagate the actions we were selecting before to the synthetic CompileJobAction that accompanies.

@graydon graydon requested a review from jrose-apple March 5, 2018 09:06
@graydon
Copy link
Contributor Author

graydon commented Mar 5, 2018

@swift-ci please test

@jrose-apple
Copy link
Contributor

Hmm. I know the JobContext member is labeled InputActions, but why? What are the other actions that should not be propagated? Is batch mode actually using the generated PCH, or is it using the regular bridging header?

@graydon
Copy link
Contributor Author

graydon commented Mar 5, 2018

@jrose-apple Scan through ToolChains.cpp for code iterating through context.InputActions and directly cast<InputAction>()-ing them. It's an implicit invariant the codebase already seems to assume: that context.InputActions is all-and-only InputAction types. Which .. kinda makes sense: they're the Action subtype that doesn't get lowered to a corresponding Job, but is recycled between layers to designate a file-on-disk dependency. The other, non-InputAction dependencies have all been lowered to Job subclasses, that are depended-on in context.Inputs.

As for "is batch mode using the generated PCH" -- yes, the PCH-generation is treated as a Job input to the derived Job, so its output is still depended-on via its inclusion in context.Inputs. This change is just ignoring the GeneratePCHJobAction (and other non-InputAction Actions) when copying Actions from the merged-Jobs' Action's inputs to the new JobContext.

(IMO the confusing bit in this is the degree to which we have a set of dependencies among Actions-and-Actions up at the Action level, and a set of dependencies among Jobs-and-Jobs down at the job level, and also a set of dependencies between Jobs and InputActions. It would maybe make things clearer if we "lowered" all the InputActions to some designated pseudo-job class called InputJob that represented a file-level dependency at-the-Job-level, so we could treat all Job-level dependencies uniformly. But that's a rather larger fix. If you concur that that'd clean this stuff up, I'm happy to file a radar and get back to it!)

@jrose-apple
Copy link
Contributor

I think the main reason there's no InputJob is because Job didn't have any subclasses, and the InputJobs would need to be treated specially anyway. But I could be wrong.

Anyway, thanks for the explanation, go for it.

@graydon
Copy link
Contributor Author

graydon commented Mar 5, 2018

(@jrose-apple interpreting that as review approval, merging)

@graydon graydon merged commit e64d810 into swiftlang:master Mar 5, 2018
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