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

Use pathing jars in cli commands #21121

Merged
merged 2 commits into from
Jul 10, 2024
Merged

Use pathing jars in cli commands #21121

merged 2 commits into from
Jul 10, 2024

Conversation

hamzaremmal
Copy link
Member

No description provided.

@hamzaremmal hamzaremmal linked an issue Jul 8, 2024 that may be closed by this pull request
New classpath management system in launchers might break #21034
Closed
@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
bishabosha
bishabosha reviewed Jul 10, 2024
View reviewed changes
compiler/src/dotty/tools/dotc/classpath/ClassPathFactory.scala
@@ -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 •
edited
Loading

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 •
edited
Loading

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
24 checks passed
@hamzaremmal hamzaremmal deleted the i21034 branch July 10, 2024 15:02
@bishabosha
Copy link
Member

bishabosha commented Jul 10, 2024 •
edited
Loading

@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

@bishabosha bishabosha mentioned this pull request Jul 10, 2024
Merged
hamzaremmal added a commit that referenced this pull request Jul 10, 2024
@hamzaremmal
expand classpath of pathing jars in scala_legacy command (#21160)
466676c 
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 WojciechMazur mentioned this pull request Jul 10, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 11, 2024
@WojciechMazur
Backport "Use pathing jars in cli commands" to 3.5.0 (#21172)
a578242 
Backports #21121 to 3.5.0-RC4
@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 WojciechMazur mentioned this pull request Jul 12, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 14, 2024
@WojciechMazur
Backport "Use pathing jars in cli commands" to 3.5.1 (#21183)
4ccb020 
Backports #21121 to 3.5.1-RC1
@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
Reviewers

@bishabosha bishabosha bishabosha left review comments

@WojciechMazur WojciechMazur WojciechMazur approved these changes

@Gedochao Gedochao Awaiting requested review from Gedochao

Assignees

@bishabosha bishabosha

@Gedochao Gedochao

@WojciechMazur WojciechMazur

Labels
backport:done This PR was successfully backported.
Projects
None yet
Milestone
3.5.1
Development

Successfully merging this pull request may close these issues.

New classpath management system in launchers might break
4 participants
@hamzaremmal @bishabosha @WojciechMazur @Gedochao