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

update logic that creating jvm with implicit classpath. #84

Closed
wants to merge 2 commits into from
Closed

update logic that creating jvm with implicit classpath. #84

wants to merge 2 commits into from

Conversation

worksoup
Copy link

@worksoup worksoup commented Feb 3, 2024

There may be flaws in this line.
I think the filter is not very necessary. But it just works, so I didn't change it a lot.
Hope this pr helps.

@worksoup
Copy link
Author

worksoup commented Feb 4, 2024

I made a mistake. I forgot the situation where maven is used.

Frist, filters are necessary if we need to avoid to add unexpected files to classpath .

Additionally, artifacts downloaded from maven will in 'jassets.'

So, when calling SimpleMavenDeployer.deploy, we should invoke DeployUtils.addToClasspath regardless of whether the artifact exists or not.

I have modified SimpleMavenDeployer.deploy in the hope that helps.

I hope it is correct.

@worksoup worksoup changed the title update logic when creating jvm with implicit classpath. update logic that creating jvm with implicit classpath. Feb 4, 2024
@astonbitecode
Copy link
Owner

Hi,

thank you for the investigation and for taking the time to submit a PR.

Regarding your first commit, the line

!jarname.contains("j4rs-") || jarname.ends_with(&j4rs_jar_to_use)

is there in order to make sure that all the artifacts that exist in the jassets are added in the classpath, except any other versions of j4rs that could be there by mistake... This is to cover the edge case that two or more different versions of j4rs exist in the jassets; we want only the correct version to be added in the classpath and we want to ignore the rest...

For the second commit, the

DeployUtils.addToClasspath(fullJarDeployPath);

is only called if the artifact does not exist, because, if the artifact exists, it is added by the rust code, when the JvmBuilder#build is called (while the builder generates the -Djava.class.path, by scanning the directory).
I don't think that adding a jar in the classpath twice is an issue, but it is not something that is really needed.

That being said, is there some specific issue you noticed and you did the investigation for the PR? If so, can you please describe it in order to further discuss and investigate?

@worksoup
Copy link
Author

worksoup commented Feb 5, 2024

You are correct. I made some incorrect assumptions, like thinking the program internally determines which JAR file to load.

In reality, besides these incorrect assumptions, I didn't encounter any issues. Perhaps we can consider a scenario where someone is using your library to wrap a Java library, concatenates both names as the project name, and then uploads it to the Maven repository. When they open their program for the second time, they might find that j4rs has excluded their JAR file...

Just a joke. Clearly, it's unlikely to happen, and my code doesn't handle such case. So, just close this pull request.

@worksoup worksoup closed this Feb 5, 2024
@astonbitecode
Copy link
Owner

Thanks.

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