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

[#282] fork parameter #283

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

bmarwell
Copy link
Contributor

@bmarwell bmarwell commented Oct 14, 2023

Fixes #282

This PR allows building with an external groovyc compiler. Currently, the only option is to use the groovyc from either ${GROOVY_HOME}/bin or ${PATH} (in that order), just like fork on maven-compiler-plugin works.

However, not all options can be supported. For now, I'd say let's print a warning for each of those unsupported options until groovyc does support them (we would need to track it using tickets).

Another 2nd step is to implement toolchain support as requested in #43. I consider this PR a prerequisite, prior to implementing toolchains as we would need to fork anyway when using groovyc. But using the library might be possible, too.

@bmarwell bmarwell marked this pull request as ready for review October 14, 2023 20:29
@bmarwell
Copy link
Contributor Author

This requires groovyc to be installed, e.g. via https://github.com/sdkman/sdkman-action

@bmarwell bmarwell force-pushed the feature/#282_fork branch 7 times, most recently from bfb376f to baaff2a Compare October 15, 2023 11:21
@bmarwell
Copy link
Contributor Author

Missing options requested: https://issues.apache.org/jira/browse/GROOVY-11194

@keeganwitt
Copy link
Member

This requires groovyc to be installed, e.g. via https://github.com/sdkman/sdkman-action

I think for most folks, having Groovy installed on the system would be an unusual setup, so supporting using Groovy from a dependency is critical.

@bmarwell
Copy link
Contributor Author

This requires groovyc to be installed, e.g. via https://github.com/sdkman/sdkman-action

I think for most folks, having Groovy installed on the system would be an unusual setup, so supporting using Groovy from a dependency is critical.

But that's exactly the point of the fork option. Start a new process to use an external groovyc compiler. If you want to have it downloaded via the build, there's a toolchains-maven-plugin for that.

How else would you execute the groovy compiler with a different java version, if not with groovyc as requested in the toolchain issue?

@bmarwell bmarwell requested a review from keeganwitt October 21, 2023 11:45
@keeganwitt
Copy link
Member

This is missing joint compilation options. I think we'd add that then remove the stub generation mojos?

@keeganwitt
Copy link
Member

I'm thinking maybe we should merge this to a feature branch. There's gonna be multiple steps before this is functional for users.

@bmarwell
Copy link
Contributor Author

I disagree. This is a working feature by its own. It does not have the same options as the "default" way to compile things YET, but we can ship this with updated documentation (»fork mode does not allow some options at the moment«), but ship them later instead.

If we wait for the feature to be "done", no one can profit from it for another year or so.
Maybe no other options are needed for 80% of the use cases? Let's target that first and then allow "toolchains" (differen JDK/JAVA_HOME) to work.

Thus said, I'd claim it is a documentation issue. :)

@keeganwitt
Copy link
Member

keeganwitt commented Oct 23, 2023

I disagree. This is a working feature by its own. It does not have the same options as the "default" way to compile things YET, but we can ship this with updated documentation (»fork mode does not allow some options at the moment«), but ship them later instead.

If we wait for the feature to be "done", no one can profit from it for another year or so.
Maybe no other options are needed for 80% of the use cases? Let's target that first and then allow "toolchains" (differen JDK/JAVA_HOME) to work.

Thus said, I'd claim it is a documentation issue. :)

I wasn't suggesting we need full feature parity, just that joint compilation might be something to figure out first.

@bmarwell
Copy link
Contributor Author

If I see correctly, joint compilation is a new feature? That would be easy to implement, just

  • add the new parameter,
  • print a warning on the non-fork code path,
  • add --jointCompilation to the flags
  • hint the user that he can disable the compiler-plugin

@keeganwitt
Copy link
Member

If I see correctly, joint compilation is a new feature? That would be easy to implement, just

  • add the new parameter,
  • print a warning on the non-fork code path,
  • add --jointCompilation to the flags
  • hint the user that he can disable the compiler-plugin

It's not a new feature. It's basically the stub mojos. I've been trying to think if we need to enable this option if the stub mojos are in the execution plan or maybe add a new parameter that only applies if using fork...

@bmarwell
Copy link
Contributor Author

bmarwell commented Nov 6, 2023

@keeganwitt how do we continue here? Merge with not all options available, or wait for the options to appear?

Or you can create a groovyc config file.... Just add to this PR.

@keeganwitt
Copy link
Member

@keeganwitt how do we continue here? Merge with not all options available, or wait for the options to appear?

Or you can create a groovyc config file.... Just add to this PR.

I'll have a go at adding the joint compilation stuff here, then we can merge.

@keeganwitt
Copy link
Member

Sorry for the slow progress on this. It looks like we may need yet another fix upstream to be able to set the stub dir.
bmarwell#6

@bmarwell
Copy link
Contributor Author

Sorry for the slow progress on this. It looks like we may need yet another fix upstream to be able to set the stub dir. bmarwell#6

I am beginning to think if we should just write a groovyc config file instead...

@keeganwitt
Copy link
Member

Sorry for the slow progress on this. It looks like we may need yet another fix upstream to be able to set the stub dir. bmarwell#6

I am beginning to think if we should just write a groovyc config file instead...

Config scripts are unrelated to stub generation.

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.

Create a fork option to prepare toolchain support
2 participants