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

Specify classifiers instead of using os detection in netty-all #11278

Closed
wants to merge 1 commit into from

Conversation

candrews
Copy link
Contributor

@candrews candrews commented May 18, 2021

Motivation:
The OS used to build netty-all is irrelevant to the system using netty-all.

The build environment's OS should not impact the dependencies of netty-all. The OS used to build netty-all is irrelevant to the system using netty-all at runtime. So instead of using os-maven-plugin via the jni.classifiers property to set dependency classifiers, set the correct classifiers explicitly.

Modification:
Set classifiers of the dependencies in netty-all.

Result:

Fixes #11272
See #8097

@normanmaurer
Copy link
Member

@candrews I think this will not work... as for example on linux you will still enable the linux profile when depending on netty-all and so the native dns resolver for macOS will not be included. Which means when you build on linux but deploy to Mac you will run into issues imho... the same is true for building on macOS but deploy to linux

@normanmaurer
Copy link
Member

basically netty-all should "pull in" all native dependencies no matter on which system your run and build (that was how it behaved before)

@njhill
Copy link
Member

njhill commented May 18, 2021

@normanmaurer I don't follow your reasoning, the changes look right to me. If you are building with linux profile it must mean you are targeting linux for deployment, at least that's what I infer from the comments in the pom:

    <!-- The linux profile will only include the native jar for epol to the all jar.
         If you want to also include the native jar for kqueue use -Puber.
    -->

I agree that in general it would be best to just use the uber profile so that the resulting artifacts can be deployed anywhere and work with the appropriate native libs in each case.

@normanmaurer
Copy link
Member

@njhill the problem is that the profiles will be included in the pom that is deployed and so automatically activated based on the system you are pulling the dependency in

@njhill
Copy link
Member

njhill commented May 18, 2021

@normanmaurer ok I see what you mean. The changes in this PR are still "correct" just orthogonal to the problem you're describing.

Again the way the comments are worded in the pom, it sounds like the profiles are intended to be set/specified based on a target deployment environment rather than activated based on the detected build environment. If that's not correct and the profile is set based on the detected OS at build time, then it's wrong to have this profile separation in the netty-all pom at all and we should just get rid of the OS-specific (non-uber) profiles. This would essentially match the old behaviour more closely too, because the profile never came into the picture when consuming the pre-built uber jar.

Motivation:
The OS used to build netty-all is irrelevant to the system using `netty-all`.

The build environment's OS should not impact the dependencies of `netty-all`. The OS used to build netty-all is irrelevant to the system using `netty-all` at runtime. So instead of using `os-maven-plugin` via the `jni.classifiers` property to set dependency classifiers, set the correct classifiers explicitly.

Modification:
Set classifiers of the dependencies in `netty-all`.

Result:

Fixes netty#11272
See netty#8097
@candrews
Copy link
Contributor Author

It seems to me that all the profiles should be removed.

  • all native dependencies should be just dependencies and not specified inside of a profile
  • Since there's no code, the coverage and full profiles are no-ops so they can be removed

The problem with this change is that it results in dependencies for platforms other than the one being built cannot be found. For example, when I build on my Linux system, netty-all fails to build with this error:

[ERROR] Failed to execute goal on project netty-all: Could not resolve dependencies for project io.netty:netty-all:jar:4.1.65.Final-SNAPSHOT: The following artifacts could not be resolved: io.netty:netty-transport-native-kqueue:jar:osx-x86_64:4.1.65.Final-SNAPSHOT, io.netty:netty-resolver-dns-native-macos:jar:osx-x86_64:4.1.65.Final-SNAPSHOT: io.netty:netty-transport-native-kqueue:jar:osx-x86_64:4.1.65.Final-SNAPSHOT was not found in https://oss.sonatype.org/content/repositories/snapshots during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of sonatype-nexus-snapshots has elapsed or updates are forced -> [Help 1]

Which of course makes sense. If it could build, the result would be correct - but I'm not sure how to get it to build.

I've updated this PR with this change so hopefully someone else can suggest a way forward.

@candrews
Copy link
Contributor Author

I'm looking at .github/workflow and expected to see where the osx packages get built, but I'm not seeing anything. How does that work?

@normanmaurer normanmaurer added this to the 4.1.66.Final milestone May 19, 2021
@candrews
Copy link
Contributor Author

@chrisvest reading #9689 (comment) it sounds like netty uses something other than GitHub Actions to build the MacOS native libraries. How does that work?

I'm guessing a similar solution is currently in place for Windows, too, based on #11284

@chrisvest
Copy link
Contributor

@candrews I don't know how the MacOS builds work, exactly. Just that they aren't GitHub Actions.

@normanmaurer
Copy link
Member

This can be closed now with #11732 in

@normanmaurer normanmaurer removed this from the 4.1.69.Final milestone Oct 7, 2021
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.

netty-all 4.1.64 has a broken dependency graph
5 participants