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

feat: maven rockcraft plugin #728

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

vpa1977
Copy link
Contributor

@vpa1977 vpa1977 commented Oct 8, 2024

Add overrides for craft-part maven plugin to allow building Maven projects in rockcraft.

Namely:

  • Do not link /bin/java as it conflicts with the base-files_base slice

  • Restrict PATH to /usr/bin in order to avoid picking up unwanted JVM.
    User can override PATH in the plugin environment settings if /usr/bin is not sufficient.

  • Have you signed the CLA?


Add overrides for craft-part maven plugin to allow
building Maven projects in rockcraft.

Namely:
 - Do not link /bin/java as it conflicts with the base-files_base slice
 - Restrict PATH to /usr/bin in order to avoid picking up unwanted JVM.
   Use can override PATH in the plugin environment settings if /usr/bin
   is not sufficient.
@vpa1977
Copy link
Contributor Author

vpa1977 commented Oct 9, 2024

Needs ci retry for lint task: /home/runner/work/rockcraft/rockcraft/docs/reuse/tutorial/setup.rst:1: WARNING: broken link: https://multipass.run/docs/how-to-install-multipass (500 Server Error: for url: https://multipass.run/docs/how-to-install-multipass)

@vpa1977 vpa1977 marked this pull request as ready for review October 9, 2024 03:58
Maven plugin is overriden in rockcraft.
Add rockcraft-specific instructions for the plugin.
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 13, 2024
@vpa1977 vpa1977 requested a review from lengau October 13, 2024 19:40
@vpa1977
Copy link
Contributor Author

vpa1977 commented Oct 13, 2024

Note: linter exception in the logs is resolved by #732

@lengau lengau requested a review from tigarmo October 15, 2024 20:39
rockcraft/plugins/maven_plugin.py Outdated Show resolved Hide resolved
rockcraft/plugins/maven_plugin.py Show resolved Hide resolved
tests/spread/rockcraft/plugin-maven/rockcraft.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@tigarmo tigarmo left a comment

Choose a reason for hiding this comment

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

@vpa1977 thanks for the PR. The main thing that's blocking me here is that the spread test project builds fine for me without the changes, which is weird because I've seen the issues that your code aims to fix in the past too.

Since you've worked with this plugin more recently, do you know of a project that would definitely not work without this rockcraft-custom plugin?

rockcraft/plugins/maven_plugin.py Outdated Show resolved Hide resolved
tests/spread/rockcraft/plugin-maven/rockcraft.yaml Outdated Show resolved Hide resolved
tests/spread/rockcraft/plugin-maven/rockcraft.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,32 @@
name: plugin-maven
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the test. I'm trying it out here with Rockcraft 1.5.3 and this project builds fine without the changes from this PR. Do you know what's going on? Ideally we'd test a case that can only work with this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@override
def get_build_environment(self) -> dict[str, str]:
env = super().get_build_environment()
env["PATH"] = "/usr/bin"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a pretty big change that we should only do if there's absolutely no other way around it. The PATH that craft-parts builds prioritizes the install and stage dirs on purpose, to let you do things like using a custom jdk in your build.

This is needed because we need Maven to pick the build-package jdk instead of the staged jre, right? Could we configure JAVA_HOME somehow? Like, to "$(dirname $(dirname $(readlink -f $(which javac))))" (the directory that contains bin/javac)?

Copy link
Contributor Author

@vpa1977 vpa1977 Oct 22, 2024

Choose a reason for hiding this comment

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

I think there are two different problems:

  • maven project does not build because selected toolchain can not build the project. Auto detected java home solves this.
  • part build is not reproducible and the build output is not what I expected as a user, because the the build picks up different toolchains. The edge case - project fails to build at all. Limiting path to /usr/bin solves this, but not in line with other part plugins environment settings. Another approach would be to sandbox staging directories to each part and ensure that it can not access output of its own staging phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe as a bandaid just put /usr/bin in front of the PATH?

@vpa1977
Copy link
Contributor Author

vpa1977 commented Oct 22, 2024

@vpa1977 thanks for the PR. The main thing that's blocking me here is that the spread test project builds fine for me without the changes, which is weird because I've seen the issues that your code aims to fix in the past too.

Since you've worked with this plugin more recently, do you know of a project that would definitely not work without this rockcraft-custom plugin?

Please refer to CI run https://github.com/canonical/rockcraft/actions/runs/11447589661/job/31850127130?pr=738 of #738. This draft PR adds only test and it fails building maven project second time.

Do not touch path,  just put /usr/bin as a priority
@vpa1977 vpa1977 requested a review from tigarmo October 22, 2024 08:26
@tigarmo
Copy link
Collaborator

tigarmo commented Oct 22, 2024

@vpa1977 thanks for the PR. The main thing that's blocking me here is that the spread test project builds fine for me without the changes, which is weird because I've seen the issues that your code aims to fix in the past too.
Since you've worked with this plugin more recently, do you know of a project that would definitely not work without this rockcraft-custom plugin?

Please refer to CI run https://github.com/canonical/rockcraft/actions/runs/11447589661/job/31850127130?pr=738 of #738. This draft PR adds only test and it fails building maven project second time.

Ah I see, I missed the part where the second execution of rockcraft pack was the one that failed. Thanks

@override
def get_build_environment(self) -> dict[str, str]:
env = super().get_build_environment()
env["PATH"] = "/usr/bin:$PATH"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still trying to understand your concerns about setting JAVA_HOME instead of PATH. After I was able to reproduce the problem in rockcraft 1.5.3, I updated the test project to this:

# ...
parts:
  maven-sample:
    plugin: maven
    source: sample
    maven-parameters: [ "-B" ]
    stage-packages:
     - openjdk-21-jre-headless_core
     - base-files_base
    build-packages:
      - maven
      - openjdk-21-jdk-headless
    build-environment:
      - JAVA_HOME: "$(dirname $(dirname $(readlink -f $(which javac))))"
    override-build: |
      echo "JAVA_HOME: ${JAVA_HOME}"
      craftctl default
      # replace chiselled image modules with jlink output
      # in order to ensure that only java.base module is present in
      # runtime
      rm -rf out
      jlink --no-header-files --no-man-pages --strip-debug \
        --add-modules java.base \
        --output out
      cp out/lib/modules ${CRAFT_PART_INSTALL}/usr/lib/jvm/java-21-openjdk-${CRAFT_ARCH_BUILD_FOR}/lib

... that is, setting JAVA_HOME and running jlink in override-build instead of override-stage. As far as I can tell it worked - I don't have the expertise to assess the actual generated files but "hello world" is later printed just fine. In both consecutive executions of rockcraft pack the line echo "JAVA_HOME: ${JAVA_HOME}" is printing JAVA_HOME: /usr/lib/jvm/java-21-openjdk-amd64. Can you help me understand what the issue with this approach is?


docker system prune -a -f

# Rebuild the java project
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Rebuild the java project
# Rebuild the java project to confirm that it rebuilds cleanly

rm ${ROCK_FILE}

# Run the packaged project
docker run --rm $IMAGE exec /usr/lib/jvm/java-21-openjdk-amd64/bin/java -jar /jar/sample-0.0.1-SNAPSHOT.jar | MATCH "hello world"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
docker run --rm $IMAGE exec /usr/lib/jvm/java-21-openjdk-amd64/bin/java -jar /jar/sample-0.0.1-SNAPSHOT.jar | MATCH "hello world"
docker run --rm $IMAGE exec /usr/bin/java -jar /jar/sample-0.0.1-SNAPSHOT.jar | MATCH "hello world"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants