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.

Sign up for GitHub

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

Jump to bottom

fix: CPATH persistence #1014

Merged
merged 14 commits into from
Feb 12, 2025
Merged

fix: CPATH persistence #1014

merged 14 commits into from
Feb 12, 2025
+86 −6

Conversation

yanksyoon
Copy link
Contributor

@yanksyoon yanksyoon commented Feb 10, 2025 •
edited
Loading

  • Have you signed the CLA?
  • Have you added an entry to the changelog (docs/reference/changelog.rst)?

Fixes: #1013 (comment)

@yanksyoon yanksyoon marked this pull request as ready for review February 10, 2025 11:55
@vpa1977
fix: set --print-module-deps as first argument
2a05016 
A bug in jdeps  causes --print-module-deps to ignore recursive settings
when it is present on classpath after --recursive.
@vpa1977
test(jlink_plugin): add embedded jar test
d35958e
vpa1977
vpa1977 suggested changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@vpa1977 vpa1977 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

You probably want to integrate yanksyoon#1 in your PR - there is a still problem with jdeps in Java 21.
Also we need an integration test.

@yanksyoon
Merge pull request #1 from vpa1977/fix_jlink
405607b 
fix: add integration test for embedded jar
@yanksyoon yanksyoon requested review from tigarmo and vpa1977 February 11, 2025 00:31
vpa1977
vpa1977 approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@vpa1977 vpa1977 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

maybe amend changelog regarding jdeps argument order change?

tigarmo
tigarmo requested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@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.

Thanks! The code itself is fine, just a couple of points about the changelog and test

docs/reference/changelog.rst Outdated Show resolved Hide resolved
tests/integration/plugins/test_jlink.py Outdated Show resolved Hide resolved
tests/integration/plugins/test_jlink.py Outdated Show resolved Hide resolved
tests/integration/plugins/test_jlink.py Outdated Show resolved Hide resolved
@yanksyoon yanksyoon requested a review from tigarmo February 11, 2025 12:38
tigarmo
tigarmo requested changes Feb 11, 2025
View reviewed changes
docs/reference/changelog.rst Outdated Show resolved Hide resolved
tests/integration/plugins/test_jlink.py Outdated Show resolved Hide resolved
mr-cal
mr-cal approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

LGTM. The linter and integration CI failures look straightforward to resolve.

docs/reference/changelog.rst Outdated Show resolved Hide resolved
tests/integration/plugins/test_jlink.py Outdated Show resolved Hide resolved
@yanksyoon yanksyoon requested a review from tigarmo February 12, 2025 05:21
@tigarmo tigarmo merged commit d8d6836 into canonical:main Feb 12, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tigarmo tigarmo tigarmo approved these changes

@vpa1977 vpa1977 vpa1977 approved these changes

@mr-cal mr-cal mr-cal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CPATH in Jlink plugin not persisted
4 participants
@yanksyoon @tigarmo @vpa1977 @mr-cal