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 support for conditional module dependencies in jetty-start #4628

Closed
joakime opened this issue Mar 2, 2020 · 6 comments · Fixed by #4629 or #4633
Closed

Add support for conditional module dependencies in jetty-start #4628

joakime opened this issue Mar 2, 2020 · 6 comments · Fixed by #4629 or #4633
Assignees

Comments

@joakime
Copy link
Contributor

joakime commented Mar 2, 2020

Description

The jetty-start module mechanism could benefit from having [depends] on modules that may or may-not actually be present.

This would simplify behaviors like the ones identified (by @sbordet) with alpn and Java 8 update 252 and newer having built-in support for ALPN (no longer any need for alpn-boot).

Proposed syntax:

[depends]
?alpn-boot/alpn-boot-${java.version}

That would mean if the alpn-boot/alpn-boot-${java.version} module file doesn't exist, then it's a quietly skipped.
(This skipped module is still logged using StartLog's DEBUG level)

This syntax uses the "?" character to indicate an optional behavior similar to how it's used with the property syntax <name>?=<value> today.

However, the character "?" is a valid character on the filesystem. (Does this matter to us?)

@joakime joakime self-assigned this Mar 2, 2020
@sbordet
Copy link
Contributor

sbordet commented Mar 2, 2020

See #4443 for ALPN support in 8u252 without alpn-boot.

@joakime
Copy link
Contributor Author

joakime commented Mar 2, 2020

would it make more sense to have this be a different section?

[speculative]
alpn-boot/alpn-boot-${java.version}

I don't know a good name for this section as [optional] is already taken.

Some alternate thoughts on names ...

  • assume
  • rely
  • expect
  • attach
  • entrust
  • confer
  • delegate
  • furnish
  • pivot

joakime added a commit that referenced this issue Mar 2, 2020
@joakime joakime linked a pull request Mar 2, 2020 that will close this issue
@sbordet sbordet changed the title Add support for optional/speculative module dependencies in jetty-start Add support for non-required module dependencies in jetty-start Mar 3, 2020
joakime added a commit that referenced this issue Mar 3, 2020
Issue #4628 - Non-Required Module Dependency Support
@joakime
Copy link
Contributor Author

joakime commented Mar 3, 2020

Merged PR #4629

@joakime joakime closed this as completed Mar 3, 2020
joakime added a commit that referenced this issue Mar 3, 2020
@gregw gregw reopened this Mar 9, 2020
@gregw
Copy link
Contributor

gregw commented Mar 9, 2020

@sbordet How is this different from the existing [optional] mechanism?

@gregw
Copy link
Contributor

gregw commented Mar 9, 2020

@sbordet has explained the difference to [optional]. An optional dependency is just ordered but not enabled - it has to be explicitly enabled by something else.
This mechanism is for real dependencies that might not exist, but if they do exist they are definitely enabled and ordered.

Thus my issue with this is the name - NotRequired is the wrong name as it implies optional. I think Conditional better captures the meaning, as it is a real dependency that is conditional. In this case the only condition we have syntax for is existence, but in future we might add some ?{otherCondition}dependency syntax to add other conditions.

I think the naming can be fixed up in #4633

@gregw gregw changed the title Add support for non-required module dependencies in jetty-start Add support for conditional module dependencies in jetty-start Mar 10, 2020
sbordet added a commit that referenced this issue Mar 19, 2020
…y-start.

Updated after review.
Renamed isRequiredModule() -> isConditionalModule() and inverted
expressions that were using the method.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Mar 31, 2020
…on-required

Issue #4628 - Ensuring checkEnabledModules is conditional dependency aware
@sbordet
Copy link
Contributor

sbordet commented Mar 31, 2020

Fixed with #4633.

@sbordet sbordet closed this as completed Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants