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 part of the workspace baking up #965

Merged
merged 2 commits into from
Dec 6, 2017
Merged

Move part of the workspace baking up #965

merged 2 commits into from
Dec 6, 2017

Conversation

tvandijck
Copy link
Contributor

@starkos, can you think of any reason why this wouldn't be correct.
I don't see any code in the oven.bakeConfigs that actually depends on any output from projects or the bakeChildren.
I might be wrong, but I'd like to make this change if at all possible?

@starkos, can you think of any reason why this wouldn't be correct.
I don't see any code in the oven.bakeConfigs that actually depends on any output from projects or the bakeChildren.
I might be wrong, but I'd like to make this change if at all possible?
Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

LGTM, but @starkos should definitely give it a look over before it's merged.

Out of curiosity, what benefits does moving this up provide you?

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

This looks fine to me, I wouldn't anticipate any side effects. I'll let you pull the trigger on the merge though, just in case.

@tvandijck
Copy link
Contributor Author

So parts of the bakeFiles, I want to refactor (or I already did here) to do the compilebuildoutputs flag in place, rather then as a step after the p.workspace.bake. And as part of that refactor I want to add the ability for those generated files to go into another project. This would be easy if configmap wasn't a thing, but it is, and so I need to map from project config to the others project config, and the only way to do that is by mapping both from the workspace config... Unfortunately without this change, the workspace configs don't exist yet when baking the projects.

So moving this up, bakes the workspace configs first, allowing you to enumerate them during the project bake..

I may not make much sense, PR incoming..

@tvandijck
Copy link
Contributor Author

This loop in particular wouldn't work without this change:
4da400e#diff-47294235a21ef45b5104ae2328145578R615

@starkos
Copy link
Member

starkos commented Dec 6, 2017

Again, I think this looks fine. I'm hitting "Update branch" to catch it up to master, but feel free to merge when it's ready (if I don't do it first).

@tvandijck tvandijck merged commit 059d742 into master Dec 6, 2017
@tvandijck tvandijck deleted the tvandijck-patch-1 branch December 6, 2017 21:08
@tvandijck
Copy link
Contributor Author

mac is always so far behind... linux + windows are good.

@samsinsane
Copy link
Member

mac is always so far behind...

I know right! There's only a couple of hours a day where there isn't a backlog of macOS builds - they also love performing maintenance on those things all the time.

@tvandijck
Copy link
Contributor Author

Yeah, I wonder if there is an alternative, or a way to provide them with hardware for specific jobs... I don't mind setting up a mac here specifically for those builds, so we get somewhat quicker turn around...
AppVeyor is always super quick, and the Travis Linux builds are generally OK too, but I guess iOS development is just super popular, and hence always extremely backlogged.

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.

3 participants