-
Notifications
You must be signed in to change notification settings - Fork 422
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
AutoComplete: System.exit() kills maven build #596
Comments
Thanks for raising this issue. I'm thinking to address this by adding two command line flags:
The default would be to not call @bobtiernay-okta, since you raised #582, can you confirm that the above changes would still meet your requirements? |
I guess I’m not understanding why System.exit is causing an issue. If it is, can you not specify the fork option? Also, I’m partial to gnu style arguments using hyphens :) |
I updated the hyphens in the option names above. But I’m not sure if program options are the way to go. Perhaps a system property is better, to handle the case where user input was invalid: picocli may throw an error before all command line arguments are parsed. I can see @bobtiernay-okta’s use case for exit with non-zero error code on failure, and I can also see the use case for not calling It makes sense to provide some mechanism to let users control this behavior. What should this mechanism be? I can think of command line options or system properties, any other ideas? Also, what should be the default behaviour? |
@bobtiernay-okta I may use the exec goal instead of the java goal of the exec-maven-plugin to be able to fork, but this results much more configuration, because I have to create the classpath myself. @remkop I would prefer a command line option. An alternative could be to throw an exception on error, e.g. --throw-on-error. |
@markusheiden, can you clarify why you prefer a command line option?
|
@remkop As you indirectly documented above: The maven configuration would be shorter with a command line option, because there are already command line options. I can live with system properties or environment variables too, if you prefer that way. |
@markusheiden, thanks for the clarification. The problem I see with a command line option is that it is difficult to handle invalid input. For example:
Picocli will internally throw an exception when the first invalid option is seen, so before it sees the option related to exit behaviour. I'm leaning towards using a system property, since this does not have that drawback. Some possibilities:
Thoughts? |
If think it is better to let the exit codes enabled by default, because then AutoComplete can easier be used as standalone tool. So for me picocli.autocomplete.disableExitCodes, picocli.autocomplete.disableSystemExit and picocli.autocomplete.noexit are all fine. Maybe picocli.autocomplete.throwOnError would be a good option name too? Because if there is no exit code, there has to be an exception to to let the caller know about an error. |
I actually just hit this, but I didn't know it was an issue even. I just happened to look at my CI script and it hasn't been publishing since I upgraded. The reason is that it terminated before the |
@markusheiden, good point. I'll ensure that exceptions are properly propagated when @bobtiernay-okta, ok, so it looks like the default should be to not call One remaining question is whether there should be a single property to enable all Something like the below:
Thoughts? |
Guarding it behind a property which is by default off is okay, in my opinion. Especially if you provide the means to handle all error cases correctly outside of picocli. I'd drop the third option, it doesn't add a lot of value but has the potential to confuse the user "I removed the exit on error option, why is it still exiting?". Theoretically one should never call |
Just as an FYI, you can workaround this issue in Maven by using the following <plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>exec-maven-plugin</artifactId>
<version>${exec-maven-plugin.version}</version>
<executions>
<execution>
<id>generate-autocompletion-script</id>
<phase>package</phase>
<goals>
<goal>exec</goal>
</goals>
<!-- See https://picocli.info/autocomplete.html#_generate_completion_script -->
<configuration>
<executable>java</executable>
<arguments>
<argument>-cp</argument>
<classpath/>
<argument>picocli.AutoComplete</argument>
<argument>-f</argument>
<argument>-o</argument>
<argument>${project.build.directory}/my-completion.sh</argument>
<argument>my.MyCommand</argument>
</arguments>
</configuration>
</execution>
</executions>
</plugin> @remkop Perhaps we should update the docs? @markusheiden Is this really any more configuration than the <classpath/> |
@RobertZenz I agree that this should not be general Java design. But I do believe it is acceptable for CLI applications at the highest level of control for example, I always try to do something like: public final class Main {
@SuppressWarnings("PMD.DoNotCallSystemExit")
public static void main(String[] args) {
System.exit(Runner.run(args).code());
}
} If you think about this jar as a binary then this is acceptable. The only reason why this is coming up here is an implementation detail of the |
Thanks everyone for thinking together to help solve this issue! I pushed a fix to master, please verify if it matches your expectations. As part of the fix, the usage help message now has the following footer:
The only thing remaining is updating the documentation (autocomplete.adoc). It makes sense to me that we suggest to use the |
I don't think that's required for the |
@bobtiernay-okta The purpose of letting Using the I believe the So we should update the example, no? |
FYI: I have a working example. Will commit shortly. |
Docs updated. Done, closing this ticket. Thanks everyone for your help! |
Ah yes, apologies. I forgot we were changing the default behavior. Thanks! |
When using AutoComplete with the exec-maven-plugin to generate the script, the maven build process gets aborted hard, because AutoComplete uses System.exit(). So subsequent plugins (e.g. in the deploy phase) won't get executed. May you please add an option to suppress the System.exit() call?
The text was updated successfully, but these errors were encountered: