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

Add very basic support for command line only applications #1301

Closed
wants to merge 8 commits into from

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 7, 2019

Relates to: #284

This is probably wrong on a few counts since it touches core things that I'm not entirely familiar with.
I'm opening this however with the hope to get some feedback on the approach.

The basic thing the user would use is the CommandLineRunner, similar to what Spring Boot has.

I also plan on looking into integrating Aesh as mentioned by @stalep

Supersedes #1238

@geoand
Copy link
Contributor Author

geoand commented Mar 7, 2019

@stalep I have a first working example using Æsh here: https://github.com/geoand/quarkus/tree/clrunner%2Baesh (uses Æsh 2.0.0 built locally)

Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

With some discussion it was agreed that having CDI supply the main method arguments is a possible approach. What is left is a way to gracefully exit the application; this PR uses the approach of adding a new bytecode recorder when the extension is present to call a main method and exit.

Some alternative possibilities:

  • A config property that triggers immediate exit
  • Some annotation that can go on to a method which accepts a String[] and returns an int (so the user can return an exit code)
  • An alternative non-blocking variation on System.exit(int)

/**
* The bytecode is run from a main method after all RUNTIME_INIT
*/
AFTER_STARTUP
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to use an entire separate bytecode recorder just for this one case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

@geoand geoand Mar 8, 2019

Choose a reason for hiding this comment

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

I just remembered another reason why I needed this recorder - besides needing to have a way to exit. Basically the command line entry points need to be run after everything else (I am thinking basically of CDI/Arc events here).
I didn't see a way to order the execution of templates (which basically would boil down to controlling the order that calls to the templates are written to in the generated class file) - did I miss something perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, in the command line application case, it doesn't make much sense I think to show the startup time after the actual work that is intended has been performed.

So in my view the AFTER_STARTUP execution time makes sense for such applications. Is there a large performance penalty at build time of having another bytecode recorder?

Copy link
Member

Choose a reason for hiding this comment

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

It's not a large penalty by itself, but we don't want to introduce a cost if we don't need to. Since there can only ever be one of these (logically), it probably doesn't even need to be a bytecode recorder. You could for example produce a build item that contains the method info (maybe just the descriptor) to call, and then just emit a call to that method at the right spot if the build item is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! I will look into it, thanks!

Copy link
Contributor Author

@geoand geoand Mar 9, 2019

Choose a reason for hiding this comment

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

I'll give this a go to ensure that it can be implemented without too much trouble but I think there is another problem that will arise if we don't have the extra bytecode recorder.
Since we won't be able to capture arguments, we won't be able to support use cases like Aesh, were the user can write multiple commands and it would be our job to feed Aesh with the class names.

So in a non Aesh use case, yes there can only be one entry point (that makes absolute sense since it would be the equivalent of a main method) and no bytecode recorder is needed, but if we want to bring Aesh into the mix, not having one will probably be a problem.
WDYT?

Copy link
Contributor Author

@geoand geoand Mar 9, 2019

Choose a reason for hiding this comment

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

I pushed a commit that removes the need for a new BytecodeRecorder. But it does mean that now some bytecode needs to be "written" by the extension :)

Copy link
Contributor Author

@geoand geoand Mar 9, 2019

Choose a reason for hiding this comment

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

I believe that following a similar approach (where the extension actually participates in the creation of the bytecode) we could support Aesh as well

@geoand
Copy link
Contributor Author

geoand commented Mar 8, 2019

With some discussion it was agreed that having CDI supply the main method arguments is a possible approach. What is left is a way to gracefully exit the application; this PR uses the approach of adding a new bytecode recorder when the extension is present to call a main method and exit.

Some alternative possibilities:

  • A config property that triggers immediate exit
  • Some annotation that can go on to a method which accepts a String[] and returns an int (so the user can return an exit code)
  • An alternative non-blocking variation on System.exit(int)

For CDI injection of the main args, I guess changes would need to be made to Arc?

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

@geoand
Copy link
Contributor Author

geoand commented Mar 8, 2019

With some discussion it was agreed that having CDI supply the main method arguments is a possible approach. What is left is a way to gracefully exit the application; this PR uses the approach of adding a new bytecode recorder when the extension is present to call a main method and exit.
Some alternative possibilities:

  • A config property that triggers immediate exit
  • Some annotation that can go on to a method which accepts a String[] and returns an int (so the user can return an exit code)
  • An alternative non-blocking variation on System.exit(int)

For CDI injection of the main args, I guess changes would need to be made to Arc?

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

Also if the only way to inject args is via Arc, then I don't we will be able to incorporate Æsh

@dmlloyd
Copy link
Member

dmlloyd commented Mar 8, 2019

For CDI injection of the main args, I guess changes would need to be made to Arc?

I don't think so (I'm not 100% sure though). The main class would have to expose the args somehow. But it's possible that the main method build step runs too late to produce a CDI bean build item.

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

I'm not so sure. We want the user to be able to set the exit status. That's easy from an API perspective but it's difficult from the perspective that the user might be running this in dev mode, where calling System.exit() may have unpleasant effects...

@dmlloyd
Copy link
Member

dmlloyd commented Mar 8, 2019

Also if the only way to inject args is via Arc, then I don't we will be able to incorporate Æsh

Good point...

@geoand
Copy link
Contributor Author

geoand commented Mar 8, 2019

For CDI injection of the main args, I guess changes would need to be made to Arc?

I don't think so (I'm not 100% sure though). The main class would have to expose the args somehow. But it's possible that the main method build step runs too late to produce a CDI bean build item.

Of the alternatives proposed for the exit strategy, I personally feel that the dedicated annotation is more natural. In that case could the rest of the infra put in place here be used? I mean that the generated bytecode would invoke the CLI method and then attempt to shutdown in a similar manner as is done here?

I'm not so sure. We want the user to be able to set the exit status. That's easy from an API perspective but it's difficult from the perspective that the user might be running this in dev mode, where calling System.exit() may have unpleasant effects...

Does dev mode make sense for these types of applications?

@dmlloyd
Copy link
Member

dmlloyd commented Mar 8, 2019

Does dev mode make sense for these types of applications?

Perhaps not; I wonder if we can/should detect the case and give an error?

@geoand
Copy link
Contributor Author

geoand commented Mar 8, 2019

Perhaps not; I wonder if we can/should detect the case and give an error?

I was thinking the same thing.
I mean if we have an annotation that will be used to detect the method that the user intends to be used as an entry point to the command line invocation, then I would guess that we could fail when detecting such annotation and when the LaunchMode is Dev.
WDTY?

@dmlloyd
Copy link
Member

dmlloyd commented Mar 8, 2019

Seems possible, want to give it a try?

@geoand
Copy link
Contributor Author

geoand commented Mar 8, 2019

@dmlloyd Sure thing, I can't get it done today because of regular team work, but I'll try to sneak in some time over the weekend 😉

@geoand
Copy link
Contributor Author

geoand commented Mar 8, 2019

I added a commit that makes dev mode fail when a command line application is used

@geoand geoand force-pushed the cl-runner branch 3 times, most recently from 049cf23 to eb2fc23 Compare March 9, 2019 15:03
@geoand
Copy link
Contributor Author

geoand commented Mar 11, 2019

@dmlloyd Do you think that the current form of the PR makes sense as something to build on? I am thinking that if it is, I can also rebase the Aesh work I did previously in another branch.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 11, 2019

I have a few doubts. Requiring an entire extension and everything else for this seems like way too much. It would be so much better if there was just a small extra bit of code to detect an annotation and then trigger the shutdown behavior if present, skipping the extra ApplicationImpl method and all the other extra generated bytecode.

@geoand
Copy link
Contributor Author

geoand commented Mar 11, 2019

I understand the concern, indeed.
However the advantage I see with the current approach is that it can support various use cases (like the command line runner being a bean, the Aesh approach were the user would write multiple command objects).
I will of course proceed however you see fit since you have much more experience in such matters 😉

@geoand
Copy link
Contributor Author

geoand commented Mar 12, 2019

@dmlloyd would you like me to add a poc of how Aesh could be added using this approach - maybe it will help show the benefits :P

@dmlloyd
Copy link
Member

dmlloyd commented Mar 12, 2019

Sure, maybe there's something I'm not seeing about how Aesh operates.

@geoand
Copy link
Contributor Author

geoand commented Mar 12, 2019

@dmlloyd cool, I'll try to squeeze it in tonight then

@geoand
Copy link
Contributor Author

geoand commented Mar 12, 2019

@dmlloyd I added another extension that shows how Aesh could be utilized. It's pretty rudimentary, but it hopefully showcases the power of the current approach :)

It would definitely require some clean up, but I just wanted to show what's possible for the time being

@geoand geoand force-pushed the cl-runner branch 2 times, most recently from a3fc635 to a2fbce7 Compare March 12, 2019 23:17
@stalep
Copy link
Member

stalep commented Mar 13, 2019

the æsh part looks good i think. im planning on doing a pr tomorrow that will upgrade the cli to use æsh 2.0/2.1 as well so there wont be any version difference between devtools/aesh and this.

the idea of using æsh here is that the command line arguments are automatically verified and injected into the command. also, we can give the user access to a QuarkusContext object through the CommandInvocation which adds some fairly unique possibilities to access parts of quarkus without exposing any other api.

@geoand
Copy link
Contributor Author

geoand commented Mar 15, 2019

@dmlloyd Have you had a chance to look at how Aesh could benefit from the approach proposed in this PR?

@dmlloyd
Copy link
Member

dmlloyd commented Mar 18, 2019

We should talk of this in terms of requirements, which is the "hidden" step between use case and implementation. What behaviors are required for adequate Aesh integration? This would be in addition to the basic stuff:

  • Allow a program to declare that it exits immediately
  • Provide a means of controlling the process exit code for immediate-exit programs
  • Provide a means of consuming command line arguments (perhaps multiple times?)

@geoand
Copy link
Contributor Author

geoand commented Mar 18, 2019

@dmlloyd 👍

Should we setup a call perhaps with @stalep so he can give us his extensive Aesh perspective? My perspective is simply one of a Spring Boot developer used to consuming CommandLineRunner.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 18, 2019

Sure, a call sounds good. I think the Spring Boot perspective is valid also; we just want to capture any desirable aspects of that experience as usability requirements as well. Once all the requirements are in one place, implementations are easy to evaluate (and sometimes you discover new requirements in this process).

@geoand
Copy link
Contributor Author

geoand commented Mar 18, 2019

Sounds good. @dmlloyd @stalep Any preferences for when to schedule or should I just look at your calendars and find an empty slot? :)

@geoand
Copy link
Contributor Author

geoand commented Apr 10, 2019

I will close this for now. Once we have a consensus on the general strategy to be followed, I can work on it again if you like

@geoand geoand closed this Apr 10, 2019
@gsmet gsmet added the triage/invalid This doesn't seem right label Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants