intellij icon indicating copy to clipboard operation
intellij copied to clipboard

Supply code-gen rule names via settings

Open andponlin-canva opened this issue 1 year ago • 2 comments

Checklist

  • [x] I have filed an issue about this change and discussed potential changes with the maintainers.
  • [x] I have received the approval from the maintainers to make this change.
  • [x] 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.

andponlin-canva avatar Oct 13 '24 09:10 andponlin-canva

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 avatar Oct 17 '24 09:10 andponlin-canva

@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 avatar Oct 20 '24 19:10 andponlin-canva

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 avatar Oct 21 '24 10:10 andponlin-canva

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 https://github.com/bazelbuild/intellij/pull/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 https://github.com/bazelbuild/bazel/commit/35a6fc5e4db88dd3370de23e0f370608bc4e2203

cc @mai93

tpasternak avatar Oct 28 '24 13:10 tpasternak

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 avatar Oct 28 '24 13:10 tpasternak

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

andponlin-canva avatar Oct 28 '24 21:10 andponlin-canva

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

tpasternak avatar Oct 28 '24 21:10 tpasternak

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.

andponlin-canva avatar Oct 28 '24 21:10 andponlin-canva

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.

andponlin-canva avatar Oct 29 '24 06:10 andponlin-canva

Thank you Andrew!

tpasternak avatar Oct 29 '24 09:10 tpasternak