Skip to content

fix(java): adjust children module version of pom to avoid warning - #689

Open
sanyakapoor27 wants to merge 4 commits into
apache:mainfrom
sanyakapoor27:688
Open

fix(java): adjust children module version of pom to avoid warning#689
sanyakapoor27 wants to merge 4 commits into
apache:mainfrom
sanyakapoor27:688

Conversation

@sanyakapoor27

Copy link
Copy Markdown

Reason for this PR

As described in #688, this PR is done to remove the maven warnings due to version property which may break future maven builds.

What changes are included in this PR?

Removed the version property from child modules where it can be inherited from the parent and replaced expressions with constants in .

Are these changes tested?

yes, verified maven build after the changes.

Are there any user-facing changes?

no

@codecov-commenter

codecov-commenter commented May 27, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.11%. Comparing base (ba30326) to head (719f7a0).
⚠️ Report is 126 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #689       +/-   ##
===========================================
+ Coverage   59.94%   95.11%   +35.16%     
===========================================
  Files          65        8       -57     
  Lines        9213      737     -8476     
  Branches      975        0      -975     
===========================================
- Hits         5523      701     -4822     
+ Misses       3690       36     -3654     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@yecol

yecol commented May 27, 2025

Copy link
Copy Markdown
Contributor

Hi @sanyakapoor27, thanks for your commit!

However, I suggest not adding version constants in all child POMs, as this could make maintenance more difficult. It’s better to remove the version tags from child POMs.

@yecol yecol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comments.

@sanyakapoor27

Copy link
Copy Markdown
Author

Hi @yecol, thanks for the review!
I had added the constants to remove the "'version' contains an expression but should be a constant." warning. It looks like that warning might still pop up when expressions are used in child poms, but I definitely understand your concerns about maintenance. I'll make the suggested changes! :)

@yecol

yecol commented May 28, 2025

Copy link
Copy Markdown
Contributor

Thanks for the prompt update!
However, according to the build log in CI, it seems there’s still a version variable in the root POM of the maven-projects.
I believe you’ll also need to change the version in line 32 to the hardcoded value 0.12.0-SNAPSHOT.

@Thespica

Copy link
Copy Markdown
Contributor

Hi @sanyakapoor27 , Ubuntu 20.04 LTS runner has been removed on 2025-04-15(link). I will contribute a PR to fix the error. You can rebase the main branch after the updating of GitHub Action Runner : )

@sanyakapoor27

sanyakapoor27 commented May 28, 2025

Copy link
Copy Markdown
Author

@yecol thanks for the review once again! I had previously updated line 32 of the root POM to 0.12.0-SNAPSHOT, but that led to "[FATAL] Non-resolvable parent POM" errors when building the child modules.

This error and the warning mentioned in the issue #688 were gone when the < version> in the child poms also referred to 0.12.0-SNAPSHOT. This can be viewed here: Stack Overflow. In such a case, please let me know what would be the best course of action?

@yangxk1

yangxk1 commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

@sanyakapoor27 Hi! The Spark CI has been fixed, could you please rerun the CI workflow?

@yangxk1

yangxk1 commented Sep 9, 2025

Copy link
Copy Markdown
Contributor

@sanyakapoor27, do you have time to continue to complete this pr? That has not been updated for too long will be closed

@sanyakapoor27

Copy link
Copy Markdown
Author

Hi! @yangxk1 i apologise for not getting back to you sooner due to some unforseen commitments, i will update it by this weekend. thanks a lot!

@keksmd keksmd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@sanyakapoor27 I reproduced this locally end to end, so here is the full picture including an answer to your open question about Non-resolvable parent POM.

The current diff does not remove the warning

The warning is reported against the root POM's own <version>, not against the children:

[WARNING] 'version' contains an expression but should be a constant.
    @ org.apache.graphar:graphar-root:${graphar.version}, maven-projects/pom.xml, line 32, column 14

This PR only deletes <version>${graphar.version}</version> from the child modules and leaves maven-projects/pom.xml line 32 untouched, so the warning survives. Removing the redundant child <version> elements is still the right thing to do (@yecol's point about maintenance) — it just isn't sufficient on its own.

Why hardcoding line 32 alone gave you Non-resolvable parent POM

Maven resolves the <parent> coordinates before it can interpolate any properties, because properties live in the parent it hasn't loaded yet. So <parent><version> must be a literal. Every child in this repo currently has:

<parent>
    <groupId>org.apache.graphar</groupId>
    <artifactId>graphar-root</artifactId>
    <version>${graphar.version}</version>
    <relativePath>../pom.xml</relativePath>
</parent>

I reproduced your exact failure by hardcoding only line 32:

[FATAL] Non-resolvable parent POM for org.apache.graphar:graphar-info:${graphar.version}:
        The following artifacts could not be resolved:
        org.apache.graphar:graphar-root:pom:${graphar.version} (absent)

Note the coordinate: graphar-root:pom:${graphar.version} — the literal string is being used as the version. It is not a repository or connectivity problem; the parent version expression simply never expands.

The fix that works

Replace every ${graphar.version} occurrence that sits in a <version> element with the literal, in all of:

  • maven-projects/pom.xml (its own <version>, line 32)
  • maven-projects/{info,java,storage-api,spark}/pom.xml (the <parent><version>)
  • maven-projects/spark/{datasources-32,datasources-33,datasources-34,datasources-35,graphar}/pom.xml (the <parent><version>)

and drop the now-redundant child <version> elements you already removed in this PR (children inherit the parent version).

I applied that to current main and the whole reactor validates cleanly:

[INFO] Reactor Build Order:
[INFO] Building Apache GraphAr Root POM 0.13.0-SNAPSHOT   [1/7]
...
[INFO] BUILD SUCCESS

with no 'version' contains an expression warning and no Non-resolvable parent POM.

The graphar.version property itself can stay defined — it is still useful for <dependency> versions (for example maven-projects/spark/graphar/pom.xml line 42), where property interpolation works fine. Only <version> and <parent><version> elements need to be literals.

Two notes on rebasing

  1. This PR was opened against an older main. Since then maven-projects/storage-api has been added and it carries the same ${graphar.version} pattern in both places, so it needs the same treatment.
  2. There is a second, unrelated warning in the reactor that this PR should not try to absorb:
[WARNING] 'build.pluginManagement.plugins.plugin.(groupId:artifactId)' must be unique
    but found duplicate declaration of plugin org.apache.maven.plugins:maven-install-plugin
    @ line 257, column 25

Worth its own issue.

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.

6 participants