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 directory layout according to os-maven-plugin and osdetector-gradle-plugin #32

Closed
wants to merge 3 commits into from
Closed

Add directory layout according to os-maven-plugin and osdetector-gradle-plugin #32

wants to merge 3 commits into from

Conversation

bmarwell
Copy link
Contributor

@bmarwell bmarwell commented Apr 7, 2019

…e-plugin.

@bmarwell bmarwell changed the title Add directory layout accoring to os-maven-plugin and osdetector-gradl… Add directory layout according to os-maven-plugin and osdetector-gradle-plugin Apr 10, 2019
@bmarwell bmarwell closed this Aug 13, 2020
@lorenzleutgeb
Copy link

Hey! Why was this not merged? Looks really useful!

@bmarwell
Copy link
Contributor Author

Don't know. I created my own in the end which was more versatile and then didn't need it anymore. Feel free to create a new pr 😉

@imagejan
Copy link
Member

We can probably reopen this pull request if there is interest. I guess the changes proposed here aren't backward compatible, so we'd require a major version bump to 3.0.0.

@ctrueden any comments?

@tresf
Copy link
Contributor

tresf commented Jul 27, 2021

I guess the changes proposed here aren't backward compatible

They are (in theory) using the LegacyConstants class, as was intended, quoting the (non) linked issue:

[...] sort out the folder names (retaining compatibility, of course).

private static class LegacyConstants {
public static final List<String> LIST_AIX_32 = asList("aix_32");
public static final List<String> LIST_LINUX_ARM = asList("linux_arm");
public static final List<String> LIST_LINUX_32 = asList("linux_32");
public static final List<String> LIST_OSX_32 = asList("osx_32");
public static final List<String> LIST_SOLARIS_32 = asList("solaris_32");
public static final List<String> LIST_WINDOWS_32 = asList("windows_32");
public static final List<String> LIST_AIX_64 = asList("aix_64");
public static final List<String> LIST_LINUX_64 = asList("linux_64");
public static final List<String> LIST_LINUX_ARM_64 = asList("linux_arm64");
public static final List<String> LIST_OSX_64 = asList("osx_64");
public static final List<String> LIST_WINDOWS_64 = asList("windows_64");
public static final List<String> LIST_SOLARIS_64 = asList("solaris_64");
}
}

I'd also like to add that the JNA library does something very similar, so matching their exact library search patterns may be a welcomed change for any developers mixing these.

@ctrueden
Copy link
Member

ctrueden commented Jul 27, 2021

Sorry all, just too busy trying to maintain too many things. If anyone has the time and interest to help out as co-maintainer, that would be much appreciated.

I do think we should preserve backwards compatibility if possible, or else bump to version 3.0.0-SNAPSHOT according to SemVer. Does anyone have the bandwidth to review this PR and fix backwards compatibility issues? If not, I can merge it as is and bump the version. Please let me know.

@ctrueden ctrueden reopened this Jul 27, 2021
bmarwell added 3 commits July 27, 2021 11:55
In particular: os-maven-plugin and osdetector-gradle-plugin.

Signed-off-by: Curtis Rueden <[email protected]>
@ctrueden
Copy link
Member

I rebased this over the latest master. Tests still pass, but I didn't check super thoroughly that everything is 100% right.

@ctrueden
Copy link
Member

@bmarwell wrote:

I created my own in the end which was more versatile and then didn't need it anymore.

Do you think it could fully replace this library? I'm always interested in better alternatives.

@bmarwell
Copy link
Contributor Author

@bmarwell wrote:

I created my own in the end which was more versatile and then didn't need it anymore.

Do you think it could fully replace this library? I'm always interested in better alternatives.

Hi😊

In theory, yes. But nowadays I would use a completely different approach (no maps) and I never did a release. I stopped working on jssc, because I think I was a little too progressive (I wanted to avoid manual work). We also argued about caching the unpacked file between JVM starts - which is not needed in my opinion.

So, if you are interested, we could get it going again.

@tresf
Copy link
Contributor

tresf commented Jul 27, 2021

We also argued about caching the unpacked file between JVM starts - which is not needed in my opinion.

Adding context, in hindsight, the argument probably would have been better spent advocating for a simple, static location (#41) instead, as that seems how other large Java projects (IntelliJ, Cyberduck, Minecraft) handle this problem (they tackle the performance issues as a packaging/deployment task, which is probably where it belongs). My perspective has changed a bit as I work with other libraries that do the same, so, sorry. 🍻

@bmarwell
Copy link
Contributor Author

We also argued about caching the unpacked file between JVM starts - which is not needed in my opinion.

Adding context, in hindsight, the argument probably would have been better spent advocating for a simple, static location (#41) instead, as that seems how other large Java projects (IntelliJ, Cyberduck, Minecraft) handle this problem (they tackle the performance issues as a packaging/deployment task, which is probably where it belongs). My perspective has changed a bit as I work with other libraries that do the same, so, sorry.

I fully agree with that for large projects you mentioned. I am just not sure caching is so useful for a ~13kiB shared object/dll/dylib file (performance issues??). Also, having a new tmp directory each time avoids concurrency issues.
That is why I at least wanted to postpone this feature.
The performance issue does arise if you start your app multiple times per minute. Otherwise, it would probably not wear your CPU down.
If you DO have such an application, there might be a performance impact which would make caching feasible.

@ctrueden
Copy link
Member

@lorenzleutgeb Is this work something you'd still find useful? Any time and interest in testing it?

@bmarwell bmarwell closed this by deleting the head repository Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants