Skip to content

Conversation

hamzaremmal
Copy link
Member

No description provided.

@hamzaremmal hamzaremmal marked this pull request as ready for review July 10, 2024 13:08
@hamzaremmal hamzaremmal added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 10, 2024
@hamzaremmal
Copy link
Member Author

This PR should be backported 3.5.0-RC4

@WojciechMazur WojciechMazur added this to the 3.5.0 milestone Jul 10, 2024
@@ -52,14 +53,30 @@ class ClassPathFactory {

// Internal
protected def classesInPathImpl(path: String, expand: Boolean)(using Context): List[ClassPath] =
Copy link
Member

Choose a reason for hiding this comment

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

if I had one comment for cleanup - there should probably be a third argument i.e. expandManifest: Boolean and this would only be true specifically when calling classesInExpandedPath(javaUserClassPath) in PathResolvers

Copy link
Member

@bishabosha bishabosha Jul 10, 2024

Choose a reason for hiding this comment

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

which then isolates the expansion specifically to the java.class.path property - but this should not be an issue in practice

Copy link
Member

Choose a reason for hiding this comment

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

and then finally move the logic to expandPath?

Copy link
Member

@bishabosha bishabosha Jul 10, 2024

Choose a reason for hiding this comment

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

all not blockers - could be in follow up PR

@hamzaremmal hamzaremmal merged commit e5ac6c5 into scala:main Jul 10, 2024
@hamzaremmal hamzaremmal deleted the i21034 branch July 10, 2024 15:02
@bishabosha
Copy link
Member

bishabosha commented Jul 10, 2024

@unkarjedy what do you think of this alternative (using Class-Path entry of a jar manifest) to the .classpath files? - this is a java standard at least - this should be testable in the 3.5.0-RC4 artifacts when they are uploaded

hamzaremmal added a commit that referenced this pull request Jul 10, 2024
I think it still be best to delete the command, but we should ensure it
works if we're going to ship it

this should have been part of #21121
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Jul 10, 2024
WojciechMazur added a commit that referenced this pull request Jul 11, 2024
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jul 11, 2024
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:done This PR was successfully backported. labels Jul 12, 2024
@WojciechMazur WojciechMazur modified the milestones: 3.5.0, 3.5.1 Jul 12, 2024
WojciechMazur added a commit that referenced this pull request Jul 14, 2024
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New classpath management system in launchers might break
4 participants