Skip to content

extra javac opts should override default ones#1613

Merged
simuons merged 2 commits into
bazel-contrib:masterfrom
thomasbao12:tbao--fix-javac_opts_ordering
Sep 11, 2024
Merged

extra javac opts should override default ones#1613
simuons merged 2 commits into
bazel-contrib:masterfrom
thomasbao12:tbao--fix-javac_opts_ordering

Conversation

@thomasbao12

Copy link
Copy Markdown
Contributor

Description

Fixes #1550

Currently the default javac flags will be appended last, overriding the manual ones.

Motivation

@google-cla

google-cla Bot commented Sep 9, 2024

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@thomasbao12
thomasbao12 force-pushed the tbao--fix-javac_opts_ordering branch from 514ab02 to 02d001c Compare September 9, 2024 00:08
@liucijus

liucijus commented Sep 9, 2024

Copy link
Copy Markdown
Collaborator

@thomasbao12 could you add a test to prevent regressions?

@thomasbao12
thomasbao12 force-pushed the tbao--fix-javac_opts_ordering branch 2 times, most recently from b09a48b to 0fc657a Compare September 9, 2024 18:54
@thomasbao12
thomasbao12 force-pushed the tbao--fix-javac_opts_ordering branch from 0fc657a to 15f7570 Compare September 9, 2024 19:03
@thomasbao12

thomasbao12 commented Sep 9, 2024

Copy link
Copy Markdown
Contributor Author

@liucijus I added a test and verified it fails on master #1615.

I tried to follow the pattern established in other javac flag tests.

One issue though is I assume the default java toolchain sets a -target flag.

3 potential options:

  1. merge as is
  2. I could rename the test to clarify that I actually am only testing that I want to be able to override the java target version. This is actually the behavior I want to fix.
  3. I could try a more elaborate unit-test that verifies that two javac flags exist, one from the default toolchain and one from javacopts and that the javacopt one is ordered after the default toolchain.

I'm okay with 0 & 1. I worry 2 is not worth doing and overly specific.

@liucijus

Copy link
Copy Markdown
Collaborator

3 potential options:

0. merge as is

1. I could rename the test to clarify that I actually am only testing that I want to be able to override the java target version.  This is actually the behavior I want to fix.

2. I could try a more elaborate unit-test that verifies that two javac flags exist, one from the default toolchain and one from javacopts and that the javacopt one is ordered after the default toolchain.

@thomasbao12 if I haven't missed anything, option 0 seems the best to me - it communicates your need plus the name implies generic expectation

@liucijus liucijus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, @thomasbao12!

@thomasbao12

Copy link
Copy Markdown
Contributor Author

@liucijus Can you merge?

@simuons
simuons merged commit e53a43b into bazel-contrib:master Sep 11, 2024
fzakaria pushed a commit to fzakaria/rules_scala that referenced this pull request Jan 24, 2025
* extra javac opts should override default ones

* add test
fzakaria added a commit to confluentinc/rules_scala that referenced this pull request Jan 24, 2025
cherry-pick: bazelbuild#1550

Co-authored-by: thomasbao12 <thomasbao12@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

javacopts order differs from java_library

3 participants