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

Copy native libraries to source tree with Maven action executed only on explicit demand and after a test phase #153

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

pietrygamat
Copy link
Collaborator

@pietrygamat pietrygamat commented Nov 10, 2023

Following on the discussion #151 (comment) I suggest making maven the party responsible for copying newly compiled binaries to source tree from where they can be commited to git. It can be done so src/main/resources-precompiled/natives are updated only after maven is done testing. Additionally, I put the step in separate profile, so without expliclty specifying it, no src modification will take place.

@tresf
Copy link

tresf commented Nov 10, 2023

Thanks. My opinion/vote is that we keep our existing behavior and simply add the natives to the .gitignore to prevent checking into git.

The reason I prefer to keep the binary there by default is because it encourages forks to commit the natives for others to use. I realize this may seem to contradict the fact that we don't trust 3rd party libraries, but these aren't mutually exclusive. For example, if someone wants to fork and add Solaris support back in since we deliberately removed it #149, there's absolutely no guesswork as to where the native library goes.

I understand the philosophy that "polluting the source tree" is a bad thing, but these are resources -- not source -- that belong in the source tree. There were arguments to create separate repositories for these binaries, but in my opinion this really alienates contributions by "rube-goldberg-ing" everyone away with process and abstraction.

In regards to making the copy operation dependant on successful testing, this re-introduces a catch-22 that we encountered in #151 (specifically the "bootpath" test), so if we do this, we should adjust the jssc.boot.library.path unit test to copy the file from classes to a testing location, quoting:

8e6bc4a properly copies the recently compiled native library to target/classes/natives.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
@pietrygamat
Copy link
Collaborator Author

simply add the natives to the .gitignore to prevent checking into git.

no guesswork

Asking contributors to commit ignored files is a bit of guesswork on its own, right? I know fellow programmers who'd get stuck at this task ;). Note in a contribution guide telling that all is required to commit updated lib version is to add -Ppreserve-natives (or whatever name we decide on) to mvn command is quite intuitive in comparison.

Also, if we choose the .gitignore way - we must be careful not to break release pipeline, without even noticing.

copy operation dependant on successful testing re-introduces a catch-22

That's not true anymore - or at least I don't see how. The tests are executed against natives in target which are either original ones (if c compilation was skipped) or the updated ones - and the PR does not change this.

Copying libs to src now has no other purpose than having them in place for subsequent git commit, but may introduce unforseen consequences. For example, if you decide to skip c compilation and revert all your source files to clean state, you may not be aware that despite running mvn clean test -Dcmake.build.skip you are still using non-pristine lib - as you said - it's not a source after all, but a resource, and it is not open in any editor that would notify about this. Guesswork to me.

@pietrygamat pietrygamat force-pushed the copy-natives-on-demand branch from e118815 to 1feed16 Compare November 10, 2023 19:42
@tresf
Copy link

tresf commented Nov 10, 2023

Re: Catch22, I incorrectly assumed we that we were testing against the src copy. Please disregard.

Re: .gitignore it's already tracked, so my recommendation does not work, but this is a shortcoming of git. I have many projects where tracked source files should be ignore-by-default. Unfortunately, from what I can tell, this behavior doesn't exist. If it did, I'd argue it, but it doesn't so my opinion is invalid.

Re: -Pblah, I don't like it. I would rather just remove the task, I'll never remember it, so for commits such as 8b24433, I'll just copy it by hand. Historically, I'm generally the only human that actually commits these files , so if it bothers you, please remove it.

@tresf
Copy link

tresf commented Nov 10, 2023

Ooops... For automated updates (i.e. actions), we'll need something in place, edited the above.

CMakeLists.txt Outdated Show resolved Hide resolved
@tresf
Copy link

tresf commented Nov 10, 2023

Looks good. Clean and intuitive, just a minor rewording in a comment.

Co-authored-by: Tres Finocchiaro <[email protected]>
@tresf tresf merged commit 4fb5e3f into master Nov 10, 2023
26 checks passed
@tresf tresf deleted the copy-natives-on-demand branch November 10, 2023 21:00
@pietrygamat pietrygamat added this to the 2.9.6 milestone Dec 1, 2023
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.

2 participants