[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16788159#comment-16788159 ] Jeff Klukas commented on BEAM-6770: --- I plan to pick up this issue, but note I will be out of office next week, so won't begin work until the week after. > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785823#comment-16785823 ] Michael Luckey commented on BEAM-6770: -- Sure you could also add an explicit configuration. And do an e.g. 'testRuntime extends optional'. This would not help getting this into pom.xml with 'optional=true', though. And from my personal point of view, as it is *required* for running tests, maybe misleading. If it would be required for compilation, but optional for runtime, ia separate, explicitly optional configuration would make more sense, imho. And as far as I understand, this 'optional=true' is more a documentation anyhow to help aligning on a proper version, I think adding this to the doc should suffice - at least for now. With gradle 5.3 there might be a native solution to this problem in general. Lets see. > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785805#comment-16785805 ] Romain Manni-Bucau commented on BEAM-6770: -- with gradle you can always create the configuration "optional" and add it to test configuration in the build script. It is already done in some places in beam for other concerns (IIRC it was to add validates runner in test scope in runner modules). > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785798#comment-16785798 ] Ismaël Mejía commented on BEAM-6770: Yes it makes sense, if gradle does not suport optinal dependencies let's make it only runtime/testRuntime then and change the doc. Thanks [~michel] for the info. > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785789#comment-16785789 ] Jeff Klukas commented on BEAM-6770: --- Correct that the dependency is not required to compile. The class just has to be loaded by the time we call a commons-compress method that attempts to use zstd. testRuntime configuration sounds good. I'd like to avoid pulling in a new plugin to support this case. Can we rely on documenting in the Javadoc for the ZSTD enum value that it's the user's responsibility to make sure zstd-jni is loaded? > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785769#comment-16785769 ] Michael Luckey commented on BEAM-6770: -- [~jeff.klu...@gmail.com] To put it into test runtime, couldn't we simply use testRuntime configuration? IIUC the dependency is not required to compile, right? To get it into optional scope, that is not natively supported by gradle. We probably need to either run our own or use e.g. nebula.optional-base plugin [1] Did not look into the shading and problems for downstream projects consuming java-sdk-core, though. [1] https://github.com/nebula-plugins/gradle-extra-configurations-plugin > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785679#comment-16785679 ] Jeff Klukas commented on BEAM-6770: --- We did have some discussion in the PR about how the dependency should be declared and called out that making it optional was certainly a possibility we could consider. I'm happy to go that direction. We'll need to make sure zstd-jni is pulled in for tests. I'm not very familiar with Gradle, however, so it's been a struggle for me to find the right places to put these. If you have some advice on which file this dependency belongs in to make it available for tests and to mark it as an optional dependency for the Java SDK (which I don't think is strictly necessary, but perhaps is useful from a documentation standpoint), I'd appreciate the guidance. > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
[jira] [Commented] (BEAM-6770) Correct zstd-jni dependency scope to optional
[ https://issues.apache.org/jira/browse/BEAM-6770?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16785631#comment-16785631 ] Ismaël Mejía commented on BEAM-6770: It seems there is a mistake in the scope of the dependency, it must be optional because it comes as an optional dependency from commons-compress and indeed should not be mandataory for sdk-java-core execution. [~jeff.klu...@gmail.com] it seems we missed this during the review, can you PTAL? > Correct zstd-jni dependency scope to optional > - > > Key: BEAM-6770 > URL: https://issues.apache.org/jira/browse/BEAM-6770 > Project: Beam > Issue Type: Improvement > Components: sdk-java-core >Reporter: Romain Manni-Bucau >Priority: Major > > Beam 2.11.0 introduced a new transitive dep aka zstd-jni. AFAIK it is not > needed in most cases so shouldn't be here by default. Also saw it was > configured as shadow in the sdk core java module so not sure it is a gradle > build bug or intended to be like that but I think sdk-core-java should be > cleaned up cause it is now very fast and does not match a lot of usage. > Finally this lib being native it is not that sane to bring it by default, in > particular with the dockerization happening right now and the goal to have a > light container stack (which often implies to not use standard linux as FROM). -- This message was sent by Atlassian JIRA (v7.6.3#76005)
