Repository navigation
Make native libraries and the jar identify their build (#211) - #213
pietrygamat wants to merge 4 commits into
Conversation
In order to be able to identify a binary library built for any architecture,
and prevent errors when packaging during release, every native now
carries one ident line in a section of its own:
@(#)JSSC-IDENT version=${VERSION} rev=${REV} target=${LIB_DIR} origin=${ORIGIN}
cross-compile.yml passes the revision and the repository it runs in
(origin=java-native/jssc upstream, a fork reports its own name) through
-Djssc.build.rev/-Djssc.build.origin. A local build takes the revision
from git, appends -dirty when native sources have uncommitted changes,
and says origin=local.
Read it without loading the library:
- grep -a -o 'JSSC-IDENT [[:print:]]*' <lib> (any platform)
- readelf -p .jssc libjssc.so
- otool -s __TEXT __jssc libjssc.dylib (or: what libjssc.dylib)
- objdump -s -j .jssc jssc.dll
The DLLs had no version resource, so Explorer, PowerShell ((Get-Item jssc.dll).VersionInfo) and inventory tools showed nothing. FILEVERSION/PRODUCTVERSION are the numeric form of the version (2.10.5 -> 2,10,5,0). CompanyName and LegalCopyright are read by CMake from the pom (<organization> and the jssc.copyright property), so maven and plain cmake builds produce the same resource. The same copyright replaces the default javadoc footer, which named no holder, stamped the build year and said "All rights reserved". The original author is credited as a pom contributor.
|
@pietrygamat thanks for the hard work on this. What I don't completely understand is (what appears to be) the blind removal of # Drop any qualifier (2.10.5-SNAPSHOT -> 2.10.5) like release.version in pom.xml,
# so getNativeLibraryVersion() matches the Java side's LibraryVersion
string(REGEX REPLACE "-.*$" "" DETECTED_VERSION "${DETECTED_VERSION}")I haven't tested any of the code locally. With regards to the maven-template plugin, I am vehemently against anything that complicates the build process. I appreciate the list of pro's and cons there.
This opinionated/prescriptive maven (convention over configuration) system is why I constantly butt heads with other maven advocates over complicated yet overly limited plugin usage. Had I written this feature, I'd probably just write an ant task to do this for us because maven just refuses to put any custom logic in our hands with the base maven product requiring plugins upon plugins for basic tasks like this. CMake and ant are both better in this regard, in my opinion because they allow us to have fine-grained control over things like command line parsing, pattern matching, etc. I do have some outstanding maven regrets such as switching to That said, I won't hold up this PR based on this one plugin's usage. |
|
Regarding removal of We explicitly set the release version, because we wanted C api to match Java API on the x.y.z format - note SerialNativeInterface does not use SNAPSHOT now. And the The fallback in the CMake on current master is triggered only when rebuilding outside of java toolset, and since no ant that would set the version is involved - it reads it from pom.xml, and reads whatever is there, so the binary version resulting from that would NOT match the version reported by SerialInterface - that's a corner case I patch here. Not really major, we don't rely on artifacts build this way, but a smell. Regarding ant task - that's great idea that never occurred to me. I'll try to rewrite it this way, because the maven limitation in this particular case irks me too. |
Please forgive my misunderstanding here, but when I call Note that |
|
The SNAPSHOT is stripped only from a native binary, the java jar retains the SNAPSHOT, of course. A |
9074423 to
faae37a
Compare
Ah, that makes more sense... I still don't understand the purpose of this though. Don't we want to retain this information in the native binary too? |
|
I think the main reason NOT to mark native libraries as snapshots is because of how we store them. The compiled natives are currently source, not the output of the releasing build (the one that actually creates a package we publish to Maven Central and to Github releases. We need to have them committed to src before, and only set pom.xml version to non-snapshot version for exactly one git revision that the CI is creating and tagging. Alternatives I see (all out of scope of this review);
|
|
I agree that your three proposals are out of scope. I do not agree that binaries should be missing an indication of unstableness. I have a fundamental philosophy that we should never cater a build system too much to a CI because then we'll only ever have CIs producing binaries and that's not how the lifecycle of a project works. For example, if I have to patch my own copy of a DLL or SO, I should be able to tell at a glance that it's a version I built. I'm uncertain how the PR still offers this convenience. I'm not trying to minimize the problems that it brings though, so I'll test the code locally and put some thought into it. |
|
Ok, it appears SNAPSHOT was stripped back in #102, so we've stopped indicating that binaries are snapshotted. This is a regression, but one that's 5-years old. Ugh. |
That is the whole point here - the marker added in this PR adds very exact information - version (x.y.x, as in "compatible with release "x.y.z", an API level if you will) + exact git revision the code was built from + Would you want that info returned to java runtime as well? |
|
So yeah there are two workflows and we seem to have failed our users by making our maven environment assume we're always running from CI. This is not a huge-deal in practice because 99.9% of the time most users will simply take our precompiled binaries and move along, but this fails our users when they're working from a local copy.
What's not immediately obvious is how we'd ever get to 1. or 2. scenarios. Perhaps I'm missing something, but we shouldn't expect users to specify a snapshot is being created.... users should always create snapshotted versions. There have been many times that I've had to bundle a custom version of JSSC with my application and I rely on the That said, this is a 5-year old regression in behavior so it's completely reasonable to tackle this as a separate PR outside of this PR, but I don't think it's out of scope, I just think it's something that was overlooked when the automated release system was built. I guess my point is that the explicit removal of |
Hmm... how are we making any such claim? In Java with exact Java versions, |
Yeah... I think it's sane to be able to do a semantic version compare on Personally -- if I have to handle any versioning nuances -- I always do something like
(zafarkhaja/jsemver used in the above example, but any semver-compliant lib should behave identically) To be fair, I've never had to do this at the native-binary level with JSSC specifically, but |
OK, I will make that change. I'll revert cmake to read version from pom as is, and add a paramter to maven / ant build so there is a way to overwrite the version written into the bin, and we'll use it in a release event. You are right, it makes more sense anyway - as long as the
No, it means the source code in |
I had completely missed this (I saw it in my original review but it slipped my mind). This claim now makes perfect sense. Thanks for explaining. |
faae37a to
5403167
Compare
IDEs do not pick ant tasks or maven templates cleanly - I either end up with missing classes or duplicate classes until run a build with command line tools. I guess at this point it costs nothing to just add a new classpath resource file with the version hard-coded at build time and just read it. It's slight deviation I wanted to avoid - returning a constant vs reading a file, but that class is already loading resources (libs), so it should be ok. Now both getLibraryVersion and getNativeLibraryVersion use version exactly as stated in pom. |
Yeah editing source on-the-fly is messy no matter how we do it. In cmake + IDE land this is rather normal and the IDEs usually have good support for this. In mvn + IDE, I'm not so sure. I use ant for other projects and IntelliJ is pretty bad about ant support these days as they've removed the default ant plugin, I assume this is shared partially in your sentiments. It does at least prompt to install the ant-run plugin on first-run but I'm not sure how long the IDE will continue to do this. I think the current solution is good however it has a few areas of improvement.
I guess that's a lot to say that I don't think there's an easy way to tackle this problem. It does sort of make your old maven way look much cleaner in hindsight, but that's largely due to the fact that the IDEs are abandoning ant, not because its technique is any less cursed. |
SerialNativeInterface.libVersion was hardcoded and only synced by a sed step in the cross-compile workflow, so after a release it lagged behind the pom version. Move it to a LibraryVersion class that reads the version from jssc/jssc-version.properties, which maven fills in through resource filtering. getLibraryVersion() reports the jar's version as is, qualifier included (2.10.5-SNAPSHOT). The test compares it with the version surefire passes from the pom, and is skipped when run outside maven. Drop the now unnecessary workflow step.
The natives reported release.version (2.10.5-SNAPSHOT -> 2.10.5) from getNativeLibraryVersion(), in the ident and in VERSIONINFO, so a snapshot build could not be told apart from a release. They now get the pom version as is, from maven and from a plain cmake build alike, and match getLibraryVersion(). Two ant properties override it: - -Djssc.native.version=X sets it explicitly - -Djssc.native.release=true drops the qualifier (2.10.5) The release switch builds release natives while the pom still says -SNAPSHOT, ahead of release:prepare. cross-compile.yml exposes it as a "release" input, off by default.
👍
Correction, it's read once in
Pretty sure it could not be read before packaging, so it's invisible to IDE, junit tests and gets lost for downstream projects shading classes. Let's keep away.
OK, 😨 🔫 gun to your head, which will you pick? : |
5403167 to
047b049
Compare
Addresses points 2–4 of #211 (point 1 went in with #212). Every commit stands alone and covers one point, so each is explained separately below and they can be reviewed one at a time.
The goal is the one stated in #211: with the jar and its natives built at different times, on different machines and for different targets, it should be easy to answer "which build is this binary from, and does it belong with this jar?", both before publishing a release and afterwards, once the artifacts are in users' hands.
1. Embed a build ident in every native library (#211, point 3)
Why. We have no reliable way to tell which source revision and which target a
libjssc.so/.dylib/.dllwas built from. File names and directories (linux_64,linux_arm64, …) are only as trustworthy as the packaging step that put the file there, and mistakes in that step are what I want to catch before a Maven Central release. A single line in a known format, readable on every platform withgrepand without loading the library, answers this for any of our targets on whatever machine is doing the checking. This is the same idea as the package information that Fedora and other distros put into ELF objects, and as the old SCCS@(#)marker thatwhat(1)still finds.Contents and why each field is there:
version: what the native claims to be, so it can be checked against the jar.rev: the source commit. CI passes the base revision explicitly, so the ident names the same commit as thePrecompiled natives @<rev>commit rather than whatever happens to be checked out in the build container. A local build adds-dirtywhen native sources have uncommitted changes, so a hand-built binary can't pass for a clean one.target: the directory the binary is meant for. This catches a binary copied into the wrong directory, which is the packaging error that is otherwise hardest to see.origin: tells natives built by our pipeline (java-native/jssc) apart from those built by a fork's pipeline (which report the fork's own repository name) and from local builds (local).There is deliberately no build timestamp, so the output stays reproducible after #212.
Rejected alternatives
.note.packagelike the distros use. ELF-only. Mach-O and PE would each need a different mechanism and different tools. A plain named section holding a printable string works the same way for all three formats and can be found withgrep -a.getNativeLibraryVersion()alone. It has the same loading problem as a JNI function, and it carries the version only, with no revision or target.2. Add a VERSIONINFO resource to the Windows natives (#211, point 2)
Why. On Windows, the version resource is where people and tools look for this information: Explorer's Details tab,
(Get-Item jssc.dll).VersionInfo, software inventory and endpoint-security tools, installers deciding whether to overwrite a file. Our DLLs had none, so to all of these they looked like anonymous files. The ident from commit 1 is also copied intoComments, so the same build info is visible there too.Company name and copyright come from the pom (
<organization>and a newjssc.copyrightproperty). That way they are kept in one place and come out identical whether the DLL is built through Maven or with plaincmake. While setting up a single copyright value, I also used it for the javadoc footer. The default footer named no holder, stamped the build year (so the output wasn't reproducible) and said "All rights reserved", which contradicts the LGPL. The original author is now credited in the pom as a contributor, so the credit doesn't depend on the copyright string alone.3. Take the library version from the pom at build time (#211, point 4)
Why.
SerialNativeInterface.libVersionwas hardcoded and only updated by ased+ bot commit incross-compile.ymlbefore a release. After a release, the pom moves to the next SNAPSHOT but the constant doesn't, so on master the Java side and the natives report different versions (currently2.10.4vs2.10.5). That is the mismatch the version check is meant to detect, so the check becomes useless. With the value taken fromrelease.versionproject.versionat build time, it can't drift any more, and the workflow no longer needs to rewrite sources and push commits on its own.A unit test checks that the constant really was filtered (i.e. it is a plain
x.y.z, not a placeholder). This is a first, small piece of point 5.Rejected alternatives
Reading the version at runtime from a packaged resource (a filtered.propertiesfile or the jar manifest). This adds I/O and a failure mode (missing or repackaged resource, shaded jars, unusual classloaders) to code that runs while the native library loads. A compile-time constant can't go missing.sedstep and adding a second one after the release. That fixes the symptom, still keeps a version in source control that has to be kept in sync by automation, and still produces bot commits.EDIT: After some experiments, decided to read the version from a classpath resource at loadtime, rather than hardocde a constant in build-time, because: a) any load-time reading problems also affect the binary, so were are either save or already screwed anyway ; b) Maven/Ant and IDEs really hate the idea of compile time templating, and they all add a lot of friction; c) classpath resource can be read easily from renamed jar, so it's a bonus.
4. Drop the version qualifier in the CMake pom fallbackWhy. Maven passes CMakerelease.version(2.10.5-SNAPSHOT→2.10.5), and since commit 3 that is also what the Java side holds. A plaincmakebuild without Maven fell back to reading the raw pom version, so its natives reported2.10.5-SNAPSHOT(fromgetNativeLibraryVersion(), in the ident and in VERSIONINFO) and didn't match the jar they were tested with. Applying the same stripping in the fallback makes both build paths produce the same version.4. Use exact pom.xml version for both native binaries and java-side library version
Why: Since #102 we maintained the versions reported by library using getLibraryVersion and getNativeLibraryVersion using a major.minor.stamp format, and not aligning it correctly with actual pom.version. Whith the versions being now read from pom at build time, that's generally no longer necessary, with one specific exception - the pre-release cross-compile rebuild. For that, I added an option to manually overwrite the version of native library.
Not in this PR
Point 5 of #211 beyond the unit test above, i.e. a pipeline check that fails a release when a native's ident (version/target) doesn't match the jar or its directory. It builds on commits 1 and 3, so I'd rather settle the ident format here first and add the check in a follow-up.