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

Use jnr-posix-api plugin #20

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Use jnr-posix-api plugin #20

merged 1 commit into from
Jan 26, 2022

Conversation

basil
Copy link
Member

@basil basil commented Nov 28, 2021

This PR switches this plugin to depend on the newly-released jnr-posix-api plugin rather than getting JNR from core. Once this is merged and released, and once enough users have upgraded to this release, we can consider removing the JNR dependency from core.

CC @daniel-beck @Wadeck

@basil basil mentioned this pull request Nov 28, 2021
10 tasks
@daniel-beck
Copy link
Member

daniel-beck commented Dec 3, 2021

If the plugin isn't detached, wouldn't it make more sense to use a different package than hudson.os? How confident are we about classloading correctness, potentially involving dynamically loaded plugins, when classes are in core and a plugin?

@basil
Copy link
Member Author

basil commented Dec 3, 2021

How confident are we about classloading correctness, potentially involving dynamically loaded plugins, when classes are in core and a plugin?

I haven't done an end-to-end test but I'm pretty confident. I added

https://github.com/jenkinsci/jnr-posix-api-plugin/blob/fbb90b6ad669feef44cd5bcf548b8080d1e5c9d7/pom.xml#L70

to jnr-posix-api-plugin and did some testing in the script console to make sure the class loading worked as expected with both older and newer cores. Once the baseline is updated to a core that includes jenkinsci/jenkins#5979 I plan to remove everything except org.objectweb.asm. from that line.

In terms of pam-auth specifically, it doesn't have a <maskClasses> entry, so it will find the class from core right now, and once jenkinsci/jenkins#5979 is merged and released it'll fail to find it in core and then look in the plugin's class loader and find it there through jnr-posix-api-plugin. So there should be no problem.

@basil
Copy link
Member Author

basil commented Dec 22, 2021

Any next steps for moving forward with this?

@basil
Copy link
Member Author

basil commented Dec 23, 2021

@daniel-beck @Wadeck ?

@jglick
Copy link
Member

jglick commented Jan 4, 2022

it doesn't have a <maskClasses> entry, so it will find the class from core right now

I guess there is no chance of using RealJenkinsRule due to the problem of mocking out PAM?

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Sounds OK. (I apparently have GH write access, but not Artifactory access.)

@basil
Copy link
Member Author

basil commented Jan 16, 2022

@daniel-beck @Wadeck Gentle ping.

@daniel-beck
Copy link
Member

I see this updates a bundled JNA, probably due to the core dependency update and presence in the core BOM. Does this library need to be bundled here, or can we just use whatever is in core?

@basil
Copy link
Member Author

basil commented Jan 24, 2022

Does this library need to be bundled here, or can we just use whatever is in core?

JNA does not need to be bundled here, nor is it used at runtime in production because core provides JNA already. In any case this is a pre-existing issue not related to this PR.

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.

3 participants