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

feat: bazel settings integration #86

Merged
merged 1 commit into from
Sep 2, 2023

Conversation

sgammon
Copy link
Owner

@sgammon sgammon commented Sep 1, 2023

Summary

Begins integrating Bazel's built-in settings with Native Image options. First up is compilation_mode, allowing -O2 and -Ob to be passed ("optimizations on", "build speed mode on", respectively). -Ob is passed when -c fastbuild is set, and -O2 is passed when -c opt is set.

The ability to build a shared library has also been added via the new shared_library attribute. Docs have been added for both, but there is no way to reliably test these yet. Targeting #80 for release.

Related issues:

Changelog

  • feat: detect and honor compilation_mode flag
  • feat: build with debug settings if compilation_mode=dbg
  • feat: ability to build a shared library
  • feat: build with tool:coverage if coverage is enabled and a test-only target is being built
  • docs: add doc which explains shared library feature
  • docs: add doc for built settings integration (more to come)

@sgammon sgammon added feature Mainline feature work native-image Features and issues relating to the Native Image tool 🚧 WIP Work-in-progress, do not merge labels Sep 1, 2023
@sgammon sgammon added this to the 1.0.0 milestone Sep 1, 2023
@sgammon sgammon self-assigned this Sep 1, 2023
@sgammon sgammon mentioned this pull request Sep 1, 2023
@sgammon sgammon force-pushed the feat/bazel-settings-propagation branch from caf4a64 to b9635b9 Compare September 1, 2023 23:56
- feat: detect and honor `compilation_mode` flag
- feat: build with debug settings if `compilation_mode=dbg`
- feat: ability to build a shared library
- feat: build with `tool:coverage` if coverage is enabled and a
  test-only target is being built
- docs: add doc which explains shared library feature
- docs: add doc for built settings integration (more to come)

Relates-To: #78
Relates-To: #85
Signed-off-by: Sam Gammon <sam@elide.ventures>
@sonarcloud
Copy link

sonarcloud bot commented Sep 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sgammon sgammon marked this pull request as ready for review September 2, 2023 00:34
Comment on lines +5 to +10
config_setting(
name = "coverage",
values = {
"collect_code_coverage": "true",
},
)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may need to use ctx.configuration.coverage_enabled instead

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be better as it takes --instrumentation_filter into account.

Comment on lines +124 to +125
def _configure_native_test_flags(ctx, args):
"""Configure native testing flags; only applies if we are building a test-only target.
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

laying groundwork for an upcoming native_image_test rule

Comment on lines +161 to +162
if ctx.attr.shared_library:
args.add("--shared")
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: add native_image_shared_library as alias which selects header outputs automatically in addition to binary shared lib

Comment on lines +219 to +221
# append extra arguments last
for arg in ctx.attr.extra_args:
args.add(arg)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixes bug where extra args were not last

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be args.add_all, but doesn't matter too much

@sgammon sgammon merged commit bd79aa9 into release/0.10.x Sep 2, 2023
57 checks passed
run_params = {
"outputs": [binary],
"executable": graal,
"inputs": inputs,
"mnemonic": "NativeImage",
"env": native_toolchain.env,
"execution_requirements": {k: "" for k in native_toolchain.execution_requirements},
"progress_message": "Compiling native image %{label}",
"progress_message": "Native Image (__mode__) %{label}".replace("__mode__", _build_action_message(ctx)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will still end up creating a new retained string per target. Instead, you could have a dict of fixed strings per mode.

@sgammon sgammon removed the 🚧 WIP Work-in-progress, do not merge label Sep 4, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Mainline feature work native-image Features and issues relating to the Native Image tool
Projects
Development

Successfully merging this pull request may close these issues.

2 participants