Add --single-arch option to build-toolchain #82632
Conversation
MaxDesiatov left a comment
MaxDesiatov left a comment There was a problem hiding this comment.
This should not modify existing presets. And it doesn't allow using this option for build-script either. The patch application will also fail when build-presets.ini is changed. I'd recommend adding a new preset in build-presets.ini instead.
| I didn't think you'd like this approach but I avoided what you suggest as it would involve replicating a lot of text that really shouldn't be duplicated. If the script runs though it reverts the change to the presets to err on the safe-ish side. |
There's no need for duplication, since presets support mixins. What would make sense is a base preset that builds for a single platform, and an inheriting preset that avoids duplication and adds a single option to the base that enables cross-compilation for both Intel and ARM. |
| I would agree but I am not very familiar with what presets are and their syntax even. If you propose something detailed and concrete I can test it for you but would prefer not to touch the presets file at the repo level. |
They look like key/value properties where the section titles are the preset names, with special "mixin" value keys that are parsed to resolve and incorporate other preset section values (by presets.py, which also binds template text).
fwiw, the build help (in driver_arguments.py [1] EPILOG) suggests writing a custom presets file. Instead of modifying Custom presets, once written (perhaps per Max's design above) and validated, could be the basis for a proposal to add a new preset [3]. But just validating and publishing or documenting your working presets file would help others (like me) in a similar situation. (caveat: I have no authority or experience. My interest stems from a back-burner itch to get toolchain docc into universal form on macOS.) [1] [2] Use [3] A new preset would not affect any current users. Hence Max above: |
| Thanks for the documentation about presets. I agree it's difficult to even think of touching the existing presets file but I would oppose creating a new one which is the existing presets file differing by only removing the infer-cross-compile-hosts-on-darwin option as it would very quickly go out of sync. I suggest doing this in realtime as the build-toolchain script is run as an alternative. If you will never accept this then I'll close this PR. |
| You don't have to modify (the set of options in) existing presets. Adding a new base preset that existing one could mix in with a single addition of That way you're keeping the existing behavior unchanged, avoid duplication, and no longer rely on fragile hacks with patch applications that are error prone. |
Second iteration.
| I've added another commit which you could look at. It creates a new presets file with the option removed on demand and does not check it in or modify the existing presets file. As I said I would be very reluctant to "just" make changes to the existing presets as that is well beyond my pay grade. I don't think it would be easy as I'm not sure the presets used by build-toolchain and "normal" toolchain building don't overlap. |
| Please avoid creating new preset files, as those are not easily discoverable. As I mentioned above, there's an obvious way to do it that avoids duplication, doesn't change the set of options used for existing presets, doesn't introduce new options to existing build scripts, and is easy to review and to handle in the future. It is also discoverable and doesn't introduce branching out to existing workflows due to separate scripts or new preset files. The diff would be much smaller too: it literally amounts to a base preset declaration without
Would you elaborate what you mean by "normal" toolchain building? |
| By normal toolchain building I mean the CI commands which use the build-script. As I have said, I'm reluctant to modify the existing presets file for fear of regression and I don't believe it would be possible if multiple presets (with qualifiers) are shared between the various ways of generating a toolchain so they cannot inherit from a common single ancestor. I may be wrong on the latter point but I do know I don't have more time to spend exploring that and validate changes which takes hours per iteration. I'm offering a tactical fix to address the issue mentioned on the forum with very low and localised risk. |
| It's not like I don't understand what you mean it's the qualifiers that are the problem. A diff would look ike the following But that doesn't cater combinations of qualifiers like the original file. I just don't believe this is the right approach. |
| Just the first part of the diff you posted would be quite sufficient IMO. Maybe someone would want to bikeshed WRT preset naming, but I would approve this: |
| But that wouldn't be sufficient solve the problem would it? The "entry point" is preset buildbot_osx_package which is replicated along with various qualifiers like buildbot_osx_package,use_os_runtime and buildbot_osx_package,no_assertions that inherit from mixin_osx_package_base. That and the various variations per qualifier and combinations of qualifiers need to be replicated as well under a new "entry point" single_arch_osx_package (for example) inheriting from mixin_osx_package_basic as in my diff. |
Create single architecture presets.
Use single architecture preset.
| OK, I've committed a third iteration using an edit to the presets. There is only one preset that works in combination with |
| Cool. Can you kick off a CI build toolchain as a check please? |
| One more thing before that, would you mind adding a note about |
MaxDesiatov changed the title --single-arch option to build-toolchain Mention --single-arch option.
| I've added a quick note about the new option but in the longer term I wonder if the README.md of the entire project is the right place for introducing build-toolchain. Let me know if/when you want me to squash the commits. |
Co-authored-by: Max Desiatov <m_desiatov@apple.com>
| There is a problem with the presets file. It builds in the wrong directory, I'm looking at it. |
Co-authored-by: Max Desiatov <m_desiatov@apple.com>
| @swift-ci build toolchain |
Set build-subdir
| Think I've resolved the presets issue. Pease review what I have done carefully. You could also mention disk space in the --single-arch message in the README.md |
| Still working to get a toolchain with the presets approach out. It always fails with this error: There is something up with the new preset. It's simply not as simple as you think. |
johnno1962 changed the title --single-arch option to build-toolchain--single-arch option to build-toolchain Revert script to second iteration.
Revert.
| I've added commits to progressively roll back to the second iteration. I'm afraid I don't have the time to try to get a presets based solution which seems unduly complicated and risky and I consider inferior anyway (as it doesn't cater for the supported modifier combinations) to work. |
johnno1962 changed the title --single-arch option to build-toolchain--single-arch option to build-toolchain MaxDesiatov left a comment
MaxDesiatov left a comment There was a problem hiding this comment.
I don't think that a separate preset file in the current set up is worth it.
| The separate presets file is never checked in, it is always regenerated when you use the --single-arch option then deleted. Given the limitations of presets/mixins it is the easiest way to remove the option infer-cross-compile-hosts-on-darwin. |
| @airspeedswift, Any chance you could find a way to un-block this PR for me. I think it's fair to say Max and I have agreed to disagree about how to go about solving this problem building toolchains locally that came up in the forums. Hint: the solution is not to delete the build-toolchain script! |
| Sorry, but doesn't a personal custom presets file enable you to build without this PR?
That sounds like it doesn't require a PR; you'd just pass the path to your own preset file as an argument to the script. Proving that there's some preset configuration-set that builds what you want the way you want would be a first step for updating To work through your own settings or to evaluate [edit: corrected preset file name] |
| I was thinking about this impasse and I was wondering if it wasn't time to remove the infer-cross-compile-hosts-on-darwin option from the mixin_osx_package_base preset and create a new preset mixing it in for the preset the overnight toolchain build jobs use which is about the only thing that benefits from building multi-architecture now. If it is buildbot_osx_package, perhaps it would be worth creating a new preset buildbot_osx_package,multi-arch and switch the job to use that. |
3 participants
Add --single-arch option.
Hi Apple, this patch is a follow up to the conversation on S/E related to the difficulty of using the
utils/build-toolchainscript since the introduction of Macs with Apple silicon. This adds an option--single-archwhich by temporally removing theinfer-cross-compile-hosts-on-darwinoption from themixin_osx_package_basepreset allows users's to build a toolchain for their current architecture only.This should probably be the default for this script but as how it goes about it is a little untidy I'll leave it as it as an option.
Cheers