Skip to content

[build-script] Add option --infer to infer dependencies. #32256

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

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jun 9, 2020

This causes build-script to use the conservative dependency information that I
committed. Keep in mind for this to work with toolchain dependent
build-products, we need for my install-all patch to land. Once that lands
though, we have a complete solution.

@gottesmm gottesmm requested review from drexin and benlangmuir June 9, 2020 02:56
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm gottesmm force-pushed the pr-6a889299976ad0e3dfbc1849e1b7edaf177c53ce branch from 1f965d5 to 76ab50f Compare June 9, 2020 03:04
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

3 similar comments
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci smoke test

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

In a subsequent commit, I am going to make this just imply install_all since I think that is the typical behavior people will want.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

I did a little more work here and I realized I want to do install-all as part of this. Really --infer should always imply --install-all. So once I land a few other things, I am going to push the combined patch to here. Then I should be done.

@gottesmm gottesmm force-pushed the pr-6a889299976ad0e3dfbc1849e1b7edaf177c53ce branch from 76ab50f to 20318f6 Compare June 9, 2020 05:51
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

1 similar comment
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci smoke test

1 similar comment
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci smoke test

Copy link
Contributor

@drexin drexin left a comment

Choose a reason for hiding this comment

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

Nice, LGTM

This causes build-script to use the conservative dependency information that I
committed. When one uses this option, it is assumed that one wants to also
install all built products.

Some notes:

1. I included an extra --install-all option so without --infer enabled
   one can enable this sort of install everything that we want to
   build behavior.

2. I added %cmake as a lit variable. I did this so I could specify in
   my build-system unit tests that on Linux they should use the just
   built cmake (if we built cmake due to an old cmake being on the
   system). Otherwise, the build system unit tests took way too
   long. These are meant to be dry-runs, so building this cmake again
   is just wasteful and doesn't make sense.

3. I unified how we handle cmark, llvm, swift with the rest of the
   build products by making them conditional on build_* variables, but
   to preserve current behavior I made it so that they are just
   enabled by default unlike things like
   llbuild/swiftpm/foundation/etc. This was necessary since previously
   we would just pass these flags to build-script-impl and
   build-script didn't know about them. Now I taught build-script
   about them so I can manipulate these skip-build-{cmark,llvm,swift}
   and then just pass them down to build-script-impl if appropriate
   rather than relying on build-script-impl to control if these are
   built.

Once this lands, I think we are at a good enough place with
build-script until we get rid of build-script-impl in terms of high
value QoI that will imnprove productivity. Once build-script-impl is
destroyed, we can start paring back what build-script itself does.
@gottesmm gottesmm force-pushed the pr-6a889299976ad0e3dfbc1849e1b7edaf177c53ce branch from 20318f6 to a313f62 Compare June 9, 2020 18:08
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

I figured out the problem on Linux. We were trying to rebuild cmake. I fixed the code so we now use the just built one and also fixed the cmake check. We were comparing > instead of >= so even when I passed in the path to the just built cmake, we still tried to rebuild it since the version was the same = /. I added a >= and the problem went away.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

5 similar comments
@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci python lint

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@swift-ci smoke test

Copy link
Contributor

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

Having --infer imply --install-all will change the behaviour of the minimal swift build, which today does not need any installation. That may be fine, but it's not really connected to the goals of --infer. Seems worth calling out. Anyway, LGTM.

@gottesmm
Copy link
Contributor Author

gottesmm commented Jun 9, 2020

@benlangmuir it just really simplifies the model so I don't have to specify what needs to be installed or not. We could teach build-script about that if we want to. I am trying to do minimum viable product work here to get enough of these wins to tide us over until we can simplify more of this.

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