POM changes to fix build for contributors #1083
Conversation
WalkthroughThe changes across multiple Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Maven
participant Nexus
User->>Maven: Build Project
Maven->>Nexus: Deploy Artifacts
Note right of Maven: No signing/staging plugins
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Now that I see where the Github Actions are configured, I think I'll just push the next commit into this branch, disabling |
leoger
force-pushed
the
pom-fix-build-for-contributors
branch
from
3f9d516 to
81aae5b
Compare
|
NOTE: I'm leaving this in Draft state until I hear your thoughts on the basic concept and e.g. whether something like "deploy" would be a better name for the maven profile. |
|
Thanks for taking time to work on this. I'll suggest to rename the profile to - deploy as the same profile is being used for both release and snapshot builds. As for error-prone version, downgrading the version to support JDK 11 is the right way. I guess I merged the dependabot PR too quickly 😄 . |
leoger
force-pushed
the
pom-fix-build-for-contributors
branch
from
ce900e1 to
eab17b5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
.github/workflows/build.yml(4 hunks).github/workflows/release.yml(1 hunks).github/workflows/snapshot.yml(1 hunks)nitrite-bom/pom.xml(0 hunks)nitrite-jackson-mapper/pom.xml(0 hunks)nitrite-mvstore-adapter/pom.xml(0 hunks)nitrite-rocksdb-adapter/pom.xml(0 hunks)nitrite-spatial/pom.xml(0 hunks)nitrite-support/pom.xml(0 hunks)nitrite/pom.xml(0 hunks)pom.xml(2 hunks)potassium-nitrite/pom.xml(0 hunks)
💤 Files with no reviewable changes (8)
- nitrite-spatial/pom.xml
- nitrite-bom/pom.xml
- nitrite-jackson-mapper/pom.xml
- potassium-nitrite/pom.xml
- nitrite/pom.xml
- nitrite-mvstore-adapter/pom.xml
- nitrite-rocksdb-adapter/pom.xml
- nitrite-support/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
🔇 Additional comments (3)
.github/workflows/release.yml (1)
54-54: Verify profile name based on PR discussion
The addition of -P deploy aligns with moving GPG-related plugins to a profile. However, based on the PR discussion, confirm if "deploy" is the agreed-upon name (vs. the original "release" suggestion).
✅ Verification successful
Profile name 'deploy' is correctly used in the workflow
The Maven profile name -P deploy in the workflow matches the profile ID deploy defined in the root pom.xml, confirming the change is consistent with the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the profile name in pom.xml
rg -A 5 '<profile>' | rg -A 1 'id>'
Length of output: 221
.github/workflows/snapshot.yml (1)
54-54: LGTM! Consistent with release workflow
The change maintains consistency with the release workflow by using the same deploy profile, which is the correct approach as both workflows require GPG signing and artifact deployment.
.github/workflows/build.yml (1)
42-42:
Verify build behavior for contributors without GPG
The removal of -Dgpg.skip=true flags assumes the GPG plugin is properly isolated in the deploy profile. However, since the PR mentions the profile is temporarily ON by default, this could still cause build failures for contributors without GPG installed.
Consider these options:
- Keep the profile disabled by default (preferred)
- Keep
-Dgpg.skip=trueuntil the profile is confirmed to be disabled by default
Also applies to: 73-73, 96-96, 114-114
✅ Verification successful
Let me gather more information about the profile configuration.
Removal of -Dgpg.skip=true is safe for contributors
The GPG plugin is properly isolated in the deploy profile, and there's no default activation configured. The profile will only be active when explicitly enabled using -P deploy. Regular contributor builds will work without GPG as:
- The GPG plugin is strictly confined to the
deployprofile - The profile is not enabled by default (no
<activation>block) - The native tests module has GPG explicitly skipped
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the deploy profile is enabled by default in pom.xml
rg -A 10 '<profile>' | rg -A 5 'activation'
# Check for any remaining GPG plugin configurations outside the profile
rg -A 5 'maven-gpg-plugin'
Length of output: 1418
Script:
#!/bin/bash
# Check the complete profile configuration in pom.xml
rg -B 5 -A 20 '<profile>' pom.xml
Length of output: 784
|
@anidotnet any concerns about merging this PR in its current form? I have a general sense that |
|
Please remove the --% from the windows build script. As we are not using |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Removed! |
Hi @anidotnet , I already have what appears to be a working fix for #1079, but as a first step to prepare for that, I offer a proposed changes to the build setup. Basically, most people trying to follow the simple instructions in
README.mdto clone and build the project would get an error. Currently,mvn clean installrequiresgpgto be installed for the gpg signing step to run successfully.I was reading between the lines here to try to understand your intention, but I think that the configuration of the BOM module makes it very clear. Since a BOM can't truly require any meaningful build steps, the only reason to have
maven-gpg-pluginandnexus-staging-maven-pluginthere is if those are used to sign and push the actual release artifacts. If that's correct, then the traditional and recommended way to structure this within Maven is to set it up as a "profile". I put it in the parent POM and named it as "release". Then as a next step, the Github Actions config files can be updated frommvn ... deploytomvn -P release ... deploy. (NOTE: now that I look more closely at those configs, I see the existing workaround was just-Dgpg.skip=true. 😅 As I now see there is a workaround, I will no longer treat this PR as a dependency of the fix for #1079 🙏 )In order to not break any existing setups, I temporarily configured the
releaseprofile to be ON by default, but this still provides the benefit that now someone without gpg installed on their machine can build the project successfully by disabling the release profile. (That can either be set in the IDE, as in this screenshot below, or using command line flags, e.g.mvn -P '!release' <target>)The other commit in this PR is a simple version change of the error-prone plugin library. In the README file, it says:
However, the given version of the error-prone plugin fails at runtime with an
Unsupported class file major version 61error, which means it requires JDK 17, actually. You can of course tell me whether you want to hold back the error-prone version or if we should just update the README to saySummary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
error-pronedependency to ensure compatibility with JDK 11.Chores
maven-gpg-pluginandnexus-staging-maven-pluginfrom multiple projects, simplifying the build process.pom.xml.