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

Removed make_commands attribute and fixed configure_make #671

Merged
merged 2 commits into from
Jun 16, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Jun 10, 2021

Attributes which encourage users to write raw bash are generally non-hermetic and lead to a bunch of difficult edge cases to account for. The targets attribute is far more desirable since it allows the rules to provide stable and consistent APIs across all the rules.

Additionally, this change fixes an inconsistency with configure_make where the targets API was not being used.
closes #670

@UebelAndre
Copy link
Collaborator Author

I think as part of this change I should also add an example of how to have a custom make binary to replace, what seems like, a common pattern where users use make_commands to prefix or change the tool being used based on the host's configuration. This is definitely not recommended but we should find a recommended approach.

This comment describes such an approach but I'm not sure if it encompasses enough of the functionality.

@UebelAndre
Copy link
Collaborator Author

I think as part of this change I should also add an example of how to have a custom make binary to replace, what seems like, a common pattern where users use make_commands to prefix or change the tool being used based on the host's configuration. This is definitely not recommended but we should find a recommended approach.

This comment describes such an approach but I'm not sure if it encompasses enough of the functionality.

I think #676 would solve for this in a way that makes transitioning easier since it doesn't require users to setup a toolchain

@UebelAndre
Copy link
Collaborator Author

This PR is blocked until #676 is resolved

@UebelAndre UebelAndre force-pushed the target branch 2 times, most recently from 5e14552 to cd2b5d5 Compare June 14, 2021 17:19
@attilaolah
Copy link
Contributor

Cool, this is unblocked now!

By the way, once this gets merged, and now that #676 is merged too, it would be really handy to have a new release :)

@UebelAndre UebelAndre marked this pull request as ready for review June 14, 2021 17:26
@UebelAndre UebelAndre requested a review from oquenchil as a code owner June 14, 2021 17:26
@UebelAndre UebelAndre requested a review from jsharpe June 14, 2021 17:26
@UebelAndre
Copy link
Collaborator Author

@attilaolah This one is ready for review. Let me know if it solves the last issue you're having.

@attilaolah
Copy link
Contributor

I'll probably have some timet o take a look at this later today. Running some build tests now to make sure that I have a clean/passing build before patching in this one.

@attilaolah
Copy link
Contributor

OK I patched in this change, and all my builds still pass, and the targets and angs are both working as expected with configure_make(). I think this is good to go!

@UebelAndre UebelAndre enabled auto-merge (squash) June 16, 2021 13:06
@UebelAndre UebelAndre merged commit 9932c7d into bazel-contrib:main Jun 16, 2021
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.

configure_make() seems to ignore the targets attribute
2 participants