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

Update to Gradle 8; resuscitate CI #238

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

MrDOS
Copy link
Contributor

@MrDOS MrDOS commented Aug 25, 2023

This resolves #226, resolves #227, and resolves #232.

These changes specifically avoid making any meaningful changes to built artifacts. That is, this whole PR is just fixing the build process, and not changing what gets built. @madhephaestus and I talked waaay back in early 2021 about automating the release process; this isn't that. I intend to open a follow-up PR to address fully automated releases after tackling the outstanding stability issues raised in #197, #201, and #211.

I've confirmed that the newly-built native libraries pass a simple smoke test (enumerate ports) on Windows 10 on x86_64 and Linux 6.1.0/glibc 2.36 on ARM64. I'll test x86_64 macOS later today. They also work on Mac OS X 10.9 on x86_64. And FreeBSD 13.2 on ARM64.

I don't intend to commit the natives built by CI into this branch as they haven't functionally changed.

@MrDOS MrDOS requested review from madhephaestus and fwolter August 25, 2023 12:46
@MrDOS MrDOS force-pushed the update-to-gradle-8 branch from d6f1cac to 0fca43e Compare August 25, 2023 12:52
@madhephaestus
Copy link
Member

madhephaestus commented Aug 25, 2023

I have created a Repo that can publish a compiled release directly to Sonatype. If you would like to try to incorporate a release system too i can all the CI secrets to make it work. (or perhaps archive this project to move it to the active CommonWealthRobotics project that I keep up to date with release secrets).

The main reason to have a proper release in CI would be to have artifacts for all platforms generated from the tagged source, guaranteed, instead of having to commit and collect the artifacts into version control.

Thoughts?

https://github.com/CommonWealthRobotics/mujoco-java/blob/main/.github/workflows/release.yml

with:
name: freebsd
path: src/main/c/resources/native/freebsd

java:
name: Java compilation
runs-on: ubuntu-18.04
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest pinning versions, generally as it ensures that a previous build can be re-built, later in time when "latest" changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually hoping to use evergreen runner versions as a canary to help catch places where the build process accidentally relies on the default version of something in the runner image. IMO because GitHub frequently update tools within the runner images, just pinning the runner version alone doesn't guarantee much stability. I'm hoping that the other changes I've made make the runner version irrelevant to reproducibility:

  • The Linux/Windows build step now runs inside an Ubuntu 18.04 container, and so we're using the fixed version of GCC 8 shipped for that environment.
  • Same story for the FreeBSD build step; all of the meaningful work runs in the cross-compiler container.
  • The macOS build now specifically targets Mac OS X 10.7 at minimum, so (in theory) it doesn't matter what version of Xcode or macOS SDK you're using. Apple clang still happily accepts -target values for long-EOL'd OSes (right down to 10.0), so I think we should be fairly able to use just about any Xcode tools/macOS SDK and get a functionally-identical (if admittedly not byte-identical) product.
  • The Java build doesn't use any native tools at all. We configure a Java 8 JDK before we hop into Gradle, and that should be the end of the reliance on the surrounding, non-Java environment. And even if we didn't do that, the Gradle build definition specifically requires a Java 8 toolchain such that if Gradle wasn't launched by a Java 8 JDK, it will go off, download one, and use that to make the builds. (It's really neat to watch it do it, actually – launch the build from a Java 17 environment and it goes off and fetches a compiler like it would any other dependency.)

I hear where you're coming from, though. It's tedious when a PR gets held up because the build system has shifted underneath you. I can change the jobs to ubuntu-22.04/macos-12 if you'd prefer.

Copy link
Member

Choose a reason for hiding this comment

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

cool, that works, thanks.

build.gradle Show resolved Hide resolved
@MrDOS MrDOS force-pushed the update-to-gradle-8 branch from 0fca43e to ced315c Compare August 25, 2023 23:51
@MrDOS
Copy link
Contributor Author

MrDOS commented Aug 26, 2023

Thanks for taking a look at this so quickly!

I have created a Repo that can publish a compiled release directly to Sonatype.

Ah, cool, you're using the Gradle Nexus Staging plugin. I wanted to try its apparent successor, the Gradle Nexus Publish Plugin, which looks like basically the same thing, just actively maintained.

the CI secrets

When last we left off, you had provided OSSRH credentials as action secrets (OSSRHUSERNAME and OSSRHPASSWORD). I was crossing my fingers that those were still valid.

I obviously don't have the private key that you used to sign previous releases, but I don't think Sonatype/OSSRH care about that as much as they care about the release being signed by somebody. To that end, I generated a new signing keypair for [email protected], publicized it, and stored the ASCII-armoured key and its password in another pair of secrets (SIGNINGKEY and SIGNINGPASSWORD). Then in the Gradle build script, useInMemoryPgpKeys should pass those through to the signing plugin, were we ever to invoke the sign task or any of the publication tasks:

nrjavaserial/build.gradle

Lines 241 to 242 in ced315c

} else if (System.env.SIGNINGKEY && System.env.SIGNINGPASSWORD) {
useInMemoryPgpKeys(System.env.SIGNINGKEY, System.env.SIGNINGPASSWORD)

Again, haven't tried this yet, but… 🤞 Should be straightforward to invoke the publication and close/release tasks when the build workflow is triggered by a new version tag.

perhaps archive this project to move it to the active CommonWealthRobotics project

🤷‍♂️ I'm fairly ambivalent. My biggest concern would be making sure issue and PR history gets moved, and URLs get redirected appropriately. I think GitHub takes care of all that when repository ownership is transferred from one organization to another.

instead of having to commit and collect the artifacts into version control

Agreed, drives me up the wall. Would desperately love to stop dragging binaries around in Git. I've thought about writing a Gradle task to go off and fetch native libraries from the latest release or the latest CI build; that way, users still don't necessarily need a C compiler to work on the Java portion of the project.

@madhephaestus
Copy link
Member

Given the support of CI building of binaries, can we do a test release out of the PR branch?

I am happy to approve this if release should be a separate PR, but it would be nice to do have the whole build update.

@madhephaestus madhephaestus self-requested a review August 27, 2023 14:11
@MrDOS MrDOS force-pushed the update-to-gradle-8 branch 2 times, most recently from 21f422e to 407c19c Compare August 31, 2023 01:21
@MrDOS
Copy link
Contributor Author

MrDOS commented Aug 31, 2023

instead of having to commit and collect the artifacts into version control

I went down a rabbit hole for the last few days looking into writing a custom Gradle task to retrieve the native libraries from CI builds. Seems very doable! Just a bit big to include in this PR, so I'll submit that separately. The fly in the ointment is the FreeBSD arm64 library, which wasn't previously supported by CI. (I guess the rule going forward will be: no new platform support without extending the CI build. Which seems fair enough.) I've undertaken a textbook-definition yak shave and have added arm64 support to my FreeBSD cross-compilation container. Consuming that new container isn't a big change, and it's relevant to the other work being done here, so I've added that to this PR (7312c5c).

if release should be a separate PR

I'm kind of leaning in that direction; this PR has gotten quite large. But I also want to see the release automation work done too, and I also want to have confidence that it doesn't require doubling back on these changes at all. So I'm going to do that work in a separate PR, but I'll base it on this PR, and I won't merge this one until we've proven that releasing can work.

@MrDOS MrDOS force-pushed the update-to-gradle-8 branch 2 times, most recently from 3d53437 to c2a5651 Compare August 31, 2023 01:34
@madhephaestus
Copy link
Member

madhephaestus commented Aug 31, 2023 via email

Copy link
Collaborator

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

I didn't do any functional tests.

MrDOS added 11 commits September 5, 2023 13:57
Without this change, compilation on newer versions of GCC fails:

    cc1: error: ‘-mfloat-abi=hard’: selected architecture lacks an FPU

The [GCC 8 ARM changes] note that “`-mfpu=auto` is now the default
setting unless the compiler has been configured with an explicit
`--with-fpu` option.” We were setting `-mfpu` to something other than
`auto`, but one possible interpretation of that sentence is “the value
of this option is now ignored unless you pass `--with-fpu`”. And of
course, didn't, so on architectures where an FPU is optional (ARMv6 and
ARMv7), we were being pushed onto a compile path that assumed the
absence.

A quick search uncovers a few other people dealing with this. Some
distro developers discussing [how to handle the switch] refer to GCC 8,
but the general consensus seems to be that the breaking change was
introduced [with GCC 11] in Debian/Ubuntu.

The GitHub Actions runners for Ubuntu 18.04 are now dead, but the last
[installed software list] indicates that they included GCC v7.5.0,
v9.4.0, and v10.3.0; presumably v7.5.0 was the default, which is why we
didn't run into this before now. (The logs and artifacts from the last
successful CI build are long expired.)

I expect that this breaks compilation on GCC <v8.

[GCC 8 ARM changes]: https://gcc.gnu.org/gcc-8/changes.html#arm
[installed software list]: https://github.com/actions/runner-images/blob/425daf97b4452130f0065e4fc58b5c8b34ab1941/images/linux/Ubuntu1804-Readme.md
[how to handle the switch]: https://gcc.gnu.org/pipermail/gcc/2021-September/237363.html
[with GCC 11]: https://bugs.launchpad.net/ubuntu/+source/gcc-defaults/+bug/1939379#yui_3_10_3_1_1692749691943_147
The project doesn't actually use any headers from the JavaVM framework,
so we don't need to link against it.
...rather than accepting whatever default we get for the x86_64 arch.
I've chosen 10.7 because that's the lowest requirement I can find for a
Java 8 JDK. No sense in building natives for anything older if there's
no way to run them there.
GitHub will continue to merrily deprecate and remove old runner
environments, as is their prerogative. However, the simplest way to
produce a library with a low minimum glibc version requirement is to
build in an environment having an old glibc. Not only does Ubuntu 18.04
have a fairly old glibc (v2.27), but the symbol versions our library
links against require only glibc v2.7 (on x86_64; somewhat higher for
some of the other architectures).

Having previously fixed cross-compilation compatibility with GCC 8, we
are no longer able to build with GCC 7-based compilers. Thankfully,
Ubuntu did package GCC 8 on 18.04; it's just not the default.
No need to apply some of plugins outside of the plugins block.
The archive name is automatically derived from the `archivesBaseName`
and the `version`. And we don't have any duplicate archive contents, so
we don't benefit from setting a strategy to cope with them.
Some dependency configurations have been renamed as of Gradle 7.
By using the Java plugin's built-in functionality to create these
archives, they're automatically created as part of a normal build, and
included in publications.
Because signing now only happens when publishing the build and only when
signatory configuration appears to be available, we can leave the
signing block uncommented without getting in the way of people who are
just trying to make local builds.
Thanks to “toolchains”, introduced in Gradle v6.7, we can now easily
ensure that the library is always built with a Java 8 toolchain, even if
Gradle is invoked with something else.
Bnd v6 introduces a `bundle` extension to the the Java plugin's `jar`
task, which no longer relies on conventions (which are deprecated). This
mostly just clears up a deprecation warning. One warning still remains
as long as the Bnd plugin is applied, and there doesn't seem to be
anything we can do about it.
@MrDOS MrDOS force-pushed the update-to-gradle-8 branch from c2a5651 to bcb0063 Compare September 5, 2023 12:58
claui pushed a commit to dbsystel/nrjavaserial that referenced this pull request Nov 9, 2023
Usually, Gradle’s `maven-publish` plug-in is responsible for generating
a POM file dynamically at build time.

However, thanks to the excellent work of @MrDOS and others, upstream's
Gradle-6-based process is currently being replaced [1] by a
Gradle-8-based one. The latter uses a different, more convenient method
to generate the POM.

In DB Systel’s fork, we’re not going to adopt that Gradle-8-based
build process for production until upstream PR NeuronRobotics#238 is properly tested,
merged, and included in a stable upstream release.

Given that we need to get two critical bugfixes that we’ve cherry-picked
into production quickly, we’re going to accept some duplication and
commit the POM to Git for the time being.

[1]: NeuronRobotics#238
@madhephaestus
Copy link
Member

Hey all! I did some work on publishing and was able to add all the keys needed to publish directly from the CI build. The keys i added are for the whole project, so if yall wanted to incorporate the changes in the linked PR, we could begin publishing from CI FINALLY!

NeuronRobotics/JCSG#49

@MrDOS
Copy link
Contributor Author

MrDOS commented Dec 22, 2023

Amazing! I'll try to take a look soon.

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.

Compiling Problem Fresh repo clone build fails
3 participants