-
Notifications
You must be signed in to change notification settings - Fork 305
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
Supply code-gen rule names via settings #6889
Supply code-gen rule names via settings #6889
Conversation
If this is merged, please disregard PRs 6886 and 6887. I note today that on this PR there are some test issues. Aspect Tests for IJ OSS Latest Stable on :ubuntu: Ubuntu 22.04 LTSThe error in this case is;
The problem here is that the test fixture is using a Rule to test the Aspect and apparently...
I'm not sure the best path forward here as the changes in this PR are using an Aspect Parameter to convey data to the Aspect. It's a legitimate use of the Aspect Parameter but the test rig is not going to work well with it. I could go back to using an Action Env instead but I would agree that the Aspect Parameter is better. CLion OSS Latest Stable on :ubuntu: Ubuntu 22.04 LTSAppears to be an issue with Bazel 5. It's not too clear what the problem is from the trace; will require further investigation. |
@tpasternak ; I've been working on resolving problems (see earlier comment) with the testing harness out in relation to the I am making progress but the changes are not great. I am thinking that it would be better to go back to the approach of using the |
74a9b31
to
a30931a
Compare
This PR will now use |
da6111e
to
f9052e1
Compare
So, personally I'd prefer to avoid passing environment variables as it would go through all actions. That's the only issue with the PR. Thanks for looking into aspects parameters. I managed to hack that for bazel 6 and newer here #6933, however it doesn't work for bazel 5, because the change below wasn't backported there, and I'm not sure if it is possible to do it bazelbuild/bazel@35a6fc5 cc @mai93 |
I'm looking for the other possibliities. --action_env would affect the action cache, we would probably prefer to avoid that. IMO we have to either backport the change to bazel 5.x or us the aspect templating |
@tpasternak; I understand the problem with the action-env being available to all of the actions. I had considered this as a downside as well. As an alternative, would it be acceptable to revert back to use aspect parameters and make this a Bazel 7+ feature only? |
Sure, but we still have to make sure the rest of the plugin works for bazel 5 as long as it is maintained by the Bazel team |
OK; leave it with me and I will see what I can do about making this a Bazel 7+ feature and not cause any breaking problems for earlier versions. |
A prior PR has allowed that, for some languages, certain Rules may be specified as being "code generators". This mechanism was via Bazel tags on the target. This commit changes the mechanism so that the rule names are specified by settings instead.
…omments corrections" This reverts commit bffe170.
This reverts commit e1e9bd0.
f9052e1
to
246cace
Compare
… the aspect - fix android test
I reverted back to the aspect parameters approach but quickly recalled that this also doesn't work because even on Bazel 7, the test rig fails; the aspect is attached to a Rule and in this case
I've instead taken the tip above and used the aspect templating to drop a file |
Thank you Andrew! |
* Supply code-gen rule names via settings A prior PR has allowed that, for some languages, certain Rules may be specified as being "code generators". This mechanism was via Bazel tags on the target. This commit changes the mechanism so that the rule names are specified by settings instead. * Supply code-gen rule names via settings (fix android test) * Supply code-gen rule names via settings (fix android test 2) * switch the detection of code-gen rule names to action env * switch the detection of code-gen rule names to action env - comments corrections * Revert "switch the detection of code-gen rule names to action env - comments corrections" This reverts commit bffe170. * Revert "switch the detection of code-gen rule names to action env" This reverts commit e1e9bd0. * switch to use .bzl templates to convey the languages' rules' names to the aspect * switch to use .bzl templates to convey the languages' rules' names to the aspect - fix android test (cherry picked from commit 22a4474)
Checklist
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.
Discussion thread for this change
Issue number: 6725
Description of this change
Previous PRs (here and here) have been merged. This works by specifying the code-generator Bazel Rules by using a special tag and running an additional query to find the Targets with tagged rules.
This PR changes the mechanism substantially so that instead of using a tag, the list of rule names that are Python code-generator are supplied in the
.bazelproject
file like this;This approach is better because;
.bazelproject
to enable the functionality.The list of code-generator rules' names is supplied to the Bazel aspect using an aspect parameter. The aspect logic signals that it has found a code-generator using an additional boolean flag on the aspect outputs (see change to the
proto/intellij_ide_info.proto
). The plugin logic then picks up on this flag being set. Also the logic inSyncProjectTargetsHelper
is adjusted to deal with the code-generator rules' names.The example at
examples/python/simple_code_generator/...
has been adjusted so that it better demonstrates how the code-generator system works within a Python Bazel Project. The previous layout of the Targets in same Bazel Project was not revealing some of the problems that can be encountered with code-generators.The documentation has also been updated accordingly.