Skip to content

JavaKit: check each classPath element exists before starting VM #150

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

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

lhoward
Copy link
Contributor

@lhoward lhoward commented Nov 3, 2024

Partial fix for #136: validate each classPath element by attempting to open it before starting VM. It would be more efficient to stat() it but SystemPackage lacks an abstraction for checking if a file exists, and we don't want to import Foundation.

@lhoward lhoward marked this pull request as ready for review November 3, 2024 21:23
@lhoward lhoward force-pushed the lhoward/class-path-validate branch from f66e017 to fa30fb0 Compare November 3, 2024 21:24
@lhoward
Copy link
Contributor Author

lhoward commented Nov 4, 2024

Todo: check open-ing a directory is portable (specifically on Windows).

@lhoward
Copy link
Contributor Author

lhoward commented Nov 4, 2024

For sure, just thought avoiding Foundation was preferable. This won't pull in ICU?

@lhoward lhoward force-pushed the lhoward/class-path-validate branch 2 times, most recently from b4f47c3 to a35d4ab Compare November 4, 2024 09:40
@lhoward
Copy link
Contributor Author

lhoward commented Nov 4, 2024

OK, use FoundationEssentials where possible.

@lhoward lhoward force-pushed the lhoward/class-path-validate branch from a35d4ab to dcbd6b4 Compare November 5, 2024 00:15
@lhoward
Copy link
Contributor Author

lhoward commented Nov 5, 2024

  enum JavaKitError: Error {
    case classPathNotFound(String)
  } 

maybe we move VMError under this?

import FoundationEssentials
#else
import Foundation
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup that's the way, thank you

@lhoward lhoward force-pushed the lhoward/class-path-validate branch from dcbd6b4 to 4a92f99 Compare November 5, 2024 03:07
@ktoso
Copy link
Collaborator

ktoso commented Nov 5, 2024

Thanks for dealing with the nitpicks :)

@@ -268,4 +280,8 @@ extension JavaVirtualMachine {
}
}
}

enum JavaKitError: Error {
case classPathEntryNotFound(entry: String, classPath: String)
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
case classPathEntryNotFound(entry: String, classPath: String)
case classPathEntryNotFound(entry: String, classPath: [String])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixing compilation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it was supposed to be colonSeparatedClassPath – must have dropped that diff somehow. But I suppose the colon separation is an implementation detail. I'll do as you suggest above.

@ktoso
Copy link
Collaborator

ktoso commented Nov 5, 2024

If you do the "maintainers can edit" on the right I can more easily fix simple stuff like that :) There's a simple compilation error, see proposed fix.

@lhoward lhoward force-pushed the lhoward/class-path-validate branch from 4a92f99 to b08446a Compare November 5, 2024 03:29
@lhoward
Copy link
Contributor Author

lhoward commented Nov 5, 2024

If you do the "maintainers can edit" on the right I can more easily fix simple stuff like that :) There's a simple compilation error, see proposed fix.

Hmm, weirdly I can't seem to find this checkbox...

@ktoso
Copy link
Collaborator

ktoso commented Nov 5, 2024

Hmm, don't see something like that? (pic from a different PR that I made) Screenshot 2024-11-05 at 12 32 08

@ktoso ktoso merged commit 3436f46 into swiftlang:main Nov 5, 2024
11 checks passed
@ktoso
Copy link
Collaborator

ktoso commented Nov 5, 2024

Either way, thank you merged :)

@lhoward
Copy link
Contributor Author

lhoward commented Nov 5, 2024

Hmm, don't see something like that? (pic from a different PR that I made) Screenshot 2024-11-05 at 12 32 08

I think it might be because my fork was in my company repo (PADL) rather than personal. It's a bit hard to parse [1].

[1] https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#

@ktoso
Copy link
Collaborator

ktoso commented Nov 5, 2024

Huh interesting, didn't realize this limitation.

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

Successfully merging this pull request may close these issues.

2 participants