-
Notifications
You must be signed in to change notification settings - Fork 281
Add the ability to declare safe tools in a cross-build environment. #2317
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
Changes from 4 commits
a64a087
471632e
598af3c
27af6e0
3f13cb5
f7b60dd
d2dcabb
22ff484
a775dff
1d90d2e
ae60b24
81094d3
9b5e8e7
7f8ab22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ build-verbosity = 0 | |
|
||
before-all = "" | ||
before-build = "" | ||
safe-tools = [] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a default seemed "a bit too political" for @freakboy3742 in the previous PR if I recall correctly.
If we have a default then yes, that would have been my proposition.
I think that without a default, it'll try to build cmake & fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My thought here is essentially "explicit is better than implicit", for a number of reasons:
That said - consider this a "strong opinion, weakly held". If y'all would prefer to go for a list of default tools, I'm happy to oblige. And the "soft failure" approach does have some upsides: it means the ultimate failure mode for iOS builds would be the same as other platforms (i.e., no cmake installed? Error when cmake is invoked). If we do add a default set, I'd suggest:
Let me know what you'd like me to do and I'll implement that.
Sure - but a build also won't work on iOS without a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some good points above, thanks @freakboy3742, though I'm still undecided. My concern is mostly around people getting stuck. It's surprising to have PATH changed out by cibuildwheel, so when users hit the 'mybuildtool: command not found' error, do they even consider that cibuildwheel might have removed it from path? Or do they end up going down a debugging rabbit hole ("okay, i have to install cmake for some reason") before finding this option. On that note, if we do go the explicit opt-in route, rather than putting default symlinks at the start of path, we could put some entries at the bottom of PATH instead - these symlinks would run a program to print something helpful and error out:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yet another option would be to warn if no There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joerick Completely agreed that the PATH clearing is unusual/unexpected behavior, and the UX around this is important to get right. The idea of having a "dummy" tool is an interesting one - if I'm understanding you correctly, we'd use the "link all declared tools, error if they're missing"; with an additional layer that any "known tool" that isn't explicitly declared would be added as a dummy warning script. However, I think @mayeut's idea is even better. An iOS configuration must define There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so many tradeoffs! Ideally, I'd rather to avoid the concerns of complicated builds polluting the user journey of simple ones.1 Most/many builds would just set
True, but only if To stick with the current train of thought, we could soften the Lets enumerate the options...
On reflection, I'm happy with (3). The other nice thing about (3), is that the warning can say that if the build succeeds, the right value is Footnotes
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely agreed with your summary of the options; and I agree with your conclusion that (3), using a warning rather than an error is the best option. I'll implement that in the coming days (once I've worked out what is going on with the iOS CI failures reported here) |
||
repair-wheel-command = "" | ||
|
||
test-command = "" | ||
|
Uh oh!
There was an error while loading. Please reload this page.