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

Support custom mnemonic and progress_message #491

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

joshua-k-harris
Copy link

@joshua-k-harris joshua-k-harris commented Mar 6, 2024

Adds support for adding a custom mnemonic and progress_message for copy_file, copy_directory and run_binary rules.

Addresses #489 and #426

Copy link
Collaborator

@tetromino tetromino left a 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)

@joshua-k-harris joshua-k-harris force-pushed the custom-mnemonics branch 3 times, most recently from 73b8807 to 6979328 Compare March 14, 2024 16:59
@comius
Copy link
Collaborator

comius commented Mar 14, 2024

cc @brandjon

Setting mnemonics from targets is something we had huge problems with internally.

@joshua-k-harris
Copy link
Author

cc @brandjon

Setting mnemonics from targets is something we had huge problems with internally.

Does this impact the inclusion of this change or can you restrict the use of the attributes internally?

@joshua-k-harris joshua-k-harris force-pushed the custom-mnemonics branch 3 times, most recently from f7cf6a6 to 4426716 Compare April 8, 2024 10:49
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`.
@comius
Copy link
Collaborator

comius commented Jul 12, 2024

Setting mnemonics from targets is something we had huge problems with internally.

Does this impact the inclusion of this change or can you restrict the use of the attributes internally?

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 for i in range(10000000): my_macro(mnemonic=i). (Creating 1M different mnemonics happened to us). Technically Starlark already has a bag of all the literals, because it's interning them. One would need to check that mnemonics is in that bag.

This PR is acceptable after the PR in Bazel.

@sitaktif
Copy link

Thanks @comius for the context. I opened bazelbuild/bazel#23575 on the Bazel side.

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.

4 participants