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

HSC-252: Maven Parent: Set Ozone dependency in the archetype instead of the parent #63

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

rbuisson
Copy link
Contributor

@rbuisson rbuisson commented Apr 12, 2024

In order to achieve multi-level distributions, the Maven dependency on ozone must be overridden in the lower level distro to point to the higher level. Eg:

  • Ozone
    • Ozone Haiti -> depends on Ozone
      • Ozone HSC -> depends on Ozone Haiti

Some additional steps are needed to achieve multi-level distros that will be documented in a follow-up PR on Ozone Docs.
As a reminder:

  • the <id>Exclude unneeded Ozone files</id> task must be overridden at the lower level to not exclude any higher level file (if that's what's intended)
  • the <id>Unpack Ozone</id>task must be overridden to set the <includeArtifactIds></includeArtifactIds> to be the parent artifact ID. Eg: <includeArtifactIds>ozone-haiti</includeArtifactIds>

This change however introduces an important limitation: ⚠️ Ozone must be built with -P parentBuild.

This is necessary for the Maven Parent to include the necessary dependencies that have been removed by this change.

Additionally, this PR adds the assembly.xml and the corresponding build phase to the Maven Archetype and Maven Parent.

maven-parent/pom.xml Outdated Show resolved Hide resolved
@rbuisson
Copy link
Contributor Author

rbuisson commented Apr 25, 2024

I've found a better approach which consists of using Maven properties to override the Ozone Maven coordinates.
That way, a child of a child project can easily point to its parent distribution by overiding the properties:

Eg:
Ozone HSC pom.xml:

  <properties>
     ...
     <ozone.artifactId>ozone-haiti</ozone.artifactId>
     ...
  </properties>

I have not tested it yet though.

@rbuisson rbuisson requested a review from ibacher April 25, 2024 16:02
Copy link
Contributor

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

One small remark, but LGTM

<directory>${project.build.directory}/${project.artifactId}-${project.version}</directory>
</fileSet>
</fileSets>
</assembly>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
</assembly>
</assembly>

@@ -30,13 +30,16 @@

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<ozone.artifactId>ozone</ozone.artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one other place we want to use this is in the Unpack Ozone step, in the includeArtifactIds, so that we automatically unpack the artifact (it's probably fine to leave the directory things are unpacked into called "ozone".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out.
This is now fixed.

Regarding the directory names, I've updated them too so they use the provided artifact ID. That sounds much cleaner. I don't know if you think that could bring potential issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I think that's much cleaner!

@rbuisson rbuisson merged commit 384bb43 into main Apr 30, 2024
1 of 2 checks passed
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.

None yet

2 participants