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

Handle URISyntaxException #10390

Merged
merged 1 commit into from
May 3, 2023
Merged

Conversation

som-snytt
Copy link
Contributor

Minor tweak to minor code that constructs a URI/URL.

Follow-up to #10335 noticed while porting forwardly.

@scala-jenkins scala-jenkins added this to the 2.13.12 milestone May 3, 2023
@SethTisue
Copy link
Member

SethTisue commented May 3, 2023

I don't understand the motivation here — did something change in JDK 20?

@som-snytt
Copy link
Contributor Author

som-snytt commented May 3, 2023

It's trying to handle failure of URL construction, but doesn't catch URISyntaxException which is thrown by new URI. I didn't check what sorts of failures it might see IRL from jar manifests (which is calling the method).

The comment on the method that uses it:

  /** Expand manifest jar classpath entries: these are either urls, or paths
   *  relative to the location of the jar.
   */

I'm too lazy ATM to revisit the jar spec and also find out who is expanding the manifest class path, but catching the declared exception plugs the rabbit hole. If constructing the URL fails, it tries the relative path.

@SethTisue SethTisue modified the milestones: 2.13.12, 2.13.11 May 3, 2023
@SethTisue
Copy link
Member

SethTisue commented May 3, 2023

Okay. And in what user-facing context might this code be reached?

@som-snytt
Copy link
Contributor Author

OK, still within scope of laziness, I see this is ancient scala foo.jar code via paulp. I don't know whether anyone does that any more.

I had assumed incorrectly it was part of the rjolly scripting stuff for -usemanifestcp.

I know I was just looking at that jsr-223 for some reason, because I finally understood what jarlister was for.

Now I don't know what scala -usemanifestcp foo.jar does.

I also recently had a chance to laugh at test/script-tests/README.

(Also remembered that the issue with -javabootclasspath had me looking at the code. Oh, I laughed at the readme here.)

@SethTisue
Copy link
Member

SethTisue commented May 3, 2023

okay, scala foo.jar seems like sufficiently low stakes functionality that I don't need to get all paranoid. anyway, it's hard to see how this could be a bad change.

@SethTisue SethTisue merged commit 0e30133 into scala:2.13.x May 3, 2023
1 check passed
@som-snytt som-snytt deleted the tweak/uri-syntax branch May 3, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants