-
Notifications
You must be signed in to change notification settings - Fork 190
Support custom mnemonic and progress_message
#491
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
base: main
Are you sure you want to change the base?
Conversation
61f1274 to
2c79484
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable overall.
Out of curiosity, why in the macros is the order of args progress_message, mnemonic instead of the other way around (alphabetically)?
It would probably be good to add test for this. There isn't any conveniet way to test the progress message, but we can test the mnemonic via skylib's own unittest.bzl (retrieve via analysistest.target_actions(env)[0].mnemonic)
73b8807 to
6979328
Compare
|
cc @brandjon Setting mnemonics from targets is something we had huge problems with internally. |
6979328 to
f7cf6a6
Compare
Does this impact the inclusion of this change or can you restrict the use of the attributes internally? |
f7cf6a6 to
4426716
Compare
Adds support for custom 'mnemonic' and 'progress_message' attributes for 'copy_file', 'copy_directory' and 'run_binary' rules.
Adds tests to verify custom mnemonics set on `copy_file`, `copy_directory` and `run_binary`.
4426716 to
63a140f
Compare
We can't restrict them internally - we only monitor them :(. The attributes should be restricted in Bazel and this would be a very welcome contribution. The idea is, that ctx.actions.run(mnemonic) can only be set to a literal string. This way you can still use your own mnemonics, but you're more limited and you can't do This PR is acceptable after the PR in Bazel. |
|
Thanks @comius for the context. I opened bazelbuild/bazel#23575 on the Bazel side. |
Adds support for adding a custom
mnemonicandprogress_messageforcopy_file,copy_directoryandrun_binaryrules.Addresses #489 and #426