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

Release: 0.10.x #80

Merged
merged 47 commits into from
Sep 8, 2023
Merged

Release: 0.10.x #80

merged 47 commits into from
Sep 8, 2023

Conversation

sgammon
Copy link
Owner

@sgammon sgammon commented Sep 1, 2023

Summary

This branch prepares a suite of changes for collective release under the 0.10.x series. This includes several (small) backward-incompatible changes, which are documented below.

PR Tree

Dependency updates

Related issues

Breaking changes

  • The default_executable_name attribute and parameter has been renamed to executable_name.
    This is because there is no other executable name parameter; setting this attribute will unconditionally change the output binary name.

  • Legacy GVM is only usable with the legacy rules.
    Previously, it was possible to use a rather old version of GraalVM (Java 11-era) with the new-style rules. This use is no longer supported because older versions of native-image show incompatibilities with Bazel's process wrapper.

  • Native image builds will now respect Bazel flags like --compilation_mode=opt.
    Defaults have been left to GraalVM to maintain backward compatibility with the default case, which builds binaries with -O2 (optimizations on). However, this is still a breaking change because the behavior will be different if --compilation_mode was being explicitly set to any value other than opt. Note that Bazel's default value of fastbuild also applies here, so passing no value at all will result in a behavior change (optfastbuild).

  • The order of options passed to native-image has changed.
    Attributes like extra_args now pass later on the command line (last, actually, for extra_args); because native-image options are last-wins, this should allow overriding any of the flags injected by the rules. This should offer users a path forward even if the flags passed by the rules are incompatible in some way. This change in the order of action arguments may result in cache misses. Otherwise, this change should be transparent to rule users.

  • Hermetic build environment improvements.
    The parameter use_default_shell_env is no longer passed to the underlying native-image compile action; instead, the toolchain environment is assembled for strict use. Native compiler environment variables like INCLUDE, LIB, DEVELOPER_DIR, and SDKROOT no longer need to be passed via --action_env.

  • Toolchain targets and registration.
    See below for an exhaustive description, but basically, the registration calls for toolchains need to change, but only for users who are consuming via Bzlmod.

Breaking changes: Toolchain registration

When registering toolchains in a Bzlmod installation of these rules, the target @graalvm//:all must be changed to two toolchain registrations, based on the desired functionality:

Register the Java toolchain:

register_toolchains("@graalvm//:jvm")

Register the GVM toolchain:

register_toolchains("@graalvm//:sdk")

@sgammon sgammon added release Package/rules release 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 force-pushed the release/0.10.x branch 3 times, most recently from 46fca61 to 2ae10ac Compare September 1, 2023 08:30
internal/native_image/builder.bzl Show resolved Hide resolved
internal/native_image/builder.bzl Show resolved Hide resolved
internal/native_image/classic.bzl Show resolved Hide resolved
internal/native_image/common.bzl Outdated Show resolved Hide resolved
internal/native_image/rules.bzl Show resolved Hide resolved
@sgammon

This comment was marked as resolved.

@sgammon

This comment was marked as resolved.

@fmeum

This comment was marked as resolved.

@fmeum

This comment was marked as outdated.

@fmeum

This comment was marked as outdated.

@fmeum

This comment was marked as outdated.

@fmeum
Copy link
Collaborator

fmeum commented Sep 1, 2023

@sgammon Does this include the toolchain repo separation?

@sgammon
Copy link
Owner Author

sgammon commented Sep 1, 2023

@fmeum nice work, it looks like all checks are passing locally. This doesn't include the toolchain repo separation yet but it can, that change should be quick and I'd love to get it in under this release.

Copy link
Owner Author

@sgammon sgammon left a comment

Choose a reason for hiding this comment

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

Lazy toolchains

@fmeum With this change, the GVM toolchain now adopts the same structure as the Java toolchain. i couldn't figure out how to make this change without a small backward-incompatible update to the toolchain registration process, but it should be okay since we're releasing at 0.10.x, and I'll document the change in release notes.

Before 0.10.x:

flowchart TD
    A(graalvm) -->|Alias| B(graalvm_toolchain_config_repo)
    B-->C[Java toolchain]
    A-->|toolchain target|D[GVM toolchain]
Loading

After 0.10.x:

flowchart TD
    A(graalvm) -->|"Alias //:jvm"| C(graalvm_toolchain_config_repo)
    A-->|graalvm_sdk| B["//:gvm"]
    C-->|Java toolchain| D["//:toolchain"]
    A-->|"Alias //:sdk"|C
    C-->|"GVM toolchain"| E["//:gvm"]
    D-->|"SDK target"|B
Loading

This feels overly complex, but it felt wrong to define the graalvm_sdk target in the toolchain_config_repo, since those resources are actually in that repository. No matter what, I'd have to either (a) reference back to the files in a circular manner, or (b) move the GraalVM SDK call to the toolchain config repo (and reference files anyway), or (c) add a third repo, none of which seemed optimal. If you have any ideas here they are welcome.

Ideally, the actual file targets inside the GraalVM SDK could be kept private, to prevent users from binding to the internal implementation structure of the SDK, but maybe that's unavoidable or not our job to enforce.

I'm open to feedback on this and I'm curious if this solves the lazy download issue you surfaced in #66.

Inert integration tests

I've added the inert and inert-workspace tests which install @graalvm, but do not register toolchains or have native_image targets. As far as I can tell, these tests run fine and don't download GraalVM.

If you are looking to model behavior that isn't represented in these tests, please feel free to adjust them so they reflect reality.

.github/workflows/module.build.yml Show resolved Hide resolved
.github/workflows/module.build.yml Show resolved Hide resolved
.github/workflows/module.build.yml Show resolved Hide resolved
.github/workflows/module.build.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
example/integration_tests/inert/MODULE.bazel Show resolved Hide resolved
sgammon and others added 25 commits September 7, 2023 21:40
- test: add `java-toolchain` test, which exercises graalvm as the
  bazel tool/runtime java toolchain.

- test: add `components-ce` test, which downloads GVM Community
  components with dependencies.

- test: add `components-gvm` test, which downloads Oracle GVM
  components with `gu`.

- test: add `disabled_tests` with tests for earlier versions of
  bazel (coming soon), including `bazel1`, `bazel2`, and `bazel3`

- test: move most tests to GVM Community (except where noted)

Signed-off-by: Sam Gammon <[email protected]>
- 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 <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
- test: add code to `components-ce` test to ensure `js`
  component is installed and usable

Co-authored-by: Mantas Indrašius <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
- feat: add struct which defines maven coordinates for well-known
  graalvm maven dependencies

- feat: user-facing API to easily access these coordinates and
  declare them with `rules_jvm_external`

Signed-off-by: Sam Gammon <[email protected]>
- test: add integration test for artifact use
- test: add samples for each alias style
- chore: add maven artifact test to ci

Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
- fix!: toolchain repo format -> `<name>_toolchains` (previously
  was `<name>_toolchain_config_repo`)
- fix: registration of toolchain targets from workspace hook -
  use toolchain targets directly instead of `@graalvm//:*` aliases
- docs: rebuild apidoc
- chore: update bzlmod lock

Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
Signed-off-by: Sam Gammon <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Sep 8, 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 merged commit 9ef26aa into main Sep 8, 2023
63 checks passed
@sgammon sgammon removed the 🚧 WIP Work-in-progress, do not merge label Sep 8, 2023
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 release Package/rules release
Projects
2 participants