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

Supply code-gen rule names via settings #6889

Conversation

andponlin-canva
Copy link
Contributor

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

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;

python_code_generator_rule_names:
  test_codegen_files_py
  test_codegen_directory_py

This approach is better because;

  • The user only needs to specify the list of rules names in the settings and not mark each instance of the Bazel targets with the tag and specify the flag setting in the .bazelproject to enable the functionality.
  • There is no longer an additional query to search for the tag.

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 in SyncProjectTargetsHelper 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.

@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Oct 13, 2024
@andponlin-canva
Copy link
Contributor Author

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 LTS

The error in this case is;

Error in _intellij_aspect_test_fixture: \
Aspect //aspect:intellij_info.bzl%intellij_info_aspect: \
Aspect parameter attribute 'python_code_generator_rule_names' must use the 'values' restriction.

The problem here is that the test fixture is using a Rule to test the Aspect and apparently...

For rule-propagated aspects, int and string parameters must have values specified on them.

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 LTS

Appears to be an issue with Bazel 5. It's not too clear what the problem is from the trace; will require further investigation.

@andponlin-canva
Copy link
Contributor Author

@tpasternak ; I've been working on resolving problems (see earlier comment) with the testing harness out in relation to the --aspects_parameters used to convey the code generator Rules' names to the Aspect from the plugin logic.

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 --action_env instead.

@andponlin-canva andponlin-canva force-pushed the handle_py_codegen_list_of_rule_names branch from 74a9b31 to a30931a Compare October 20, 2024 19:50
@andponlin-canva
Copy link
Contributor Author

andponlin-canva commented Oct 21, 2024

This PR will now use --action_env=... instead of --aspects_parameters=... to convey the code generator Rule names to the aspect logic in order to get around problems related to needing to supply a universal set of possible values to the attr's values.

@andponlin-canva andponlin-canva force-pushed the handle_py_codegen_list_of_rule_names branch from da6111e to f9052e1 Compare October 22, 2024 00:43
@tpasternak
Copy link
Collaborator

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

@tpasternak
Copy link
Collaborator

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

@andponlin-canva
Copy link
Contributor Author

andponlin-canva commented Oct 28, 2024

@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?

@tpasternak
Copy link
Collaborator

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

@andponlin-canva
Copy link
Contributor Author

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.
@andponlin-canva andponlin-canva force-pushed the handle_py_codegen_list_of_rule_names branch from f9052e1 to 246cace Compare October 29, 2024 01:12
@andponlin-canva
Copy link
Contributor Author

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 values is required to specify a fixed set of possible parameters.

... or us the aspect templating

I've instead taken the tip above and used the aspect templating to drop a file code_generator_info.bzl into the @intellij_aspect_template containing the data. This seems to work and passes the tests.

@tpasternak
Copy link
Collaborator

Thank you Andrew!

@tpasternak tpasternak merged commit 22a4474 into bazelbuild:master Oct 29, 2024
6 checks passed
@github-actions github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Oct 29, 2024
mai93 pushed a commit that referenced this pull request Oct 30, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
Development

Successfully merging this pull request may close these issues.

4 participants