-
Notifications
You must be signed in to change notification settings - Fork 27
Allow users to specify annotated jdk path #1423
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for submitting this PR! The reason that your I think it's a reasonable next step to refine the hardcoded path in the code. One possible way could be that we copy the given jar file to checker/dist, then we can point to that fix url in the |
Are you suggesting that we should always expect users to copy their compiled annotatedJDK.jar to checker/dist? |
Yes, this could be one solution. We can also make a local copy when receiving the path to external annotated jdk. I think having a copy within the project keeps the independency, which is the same idea that we keep a copy of the annotated jdk under |
30ac2ca to
c424380
Compare
This is how I use my feature. Hi, I am not understanding why do we want to make a local copy because we already have an internal version inside checker.jar that we can use. I removed all absolute paths from my code. Now it will fallback to the internal version if it cannot find the external one. I also removed the gradle task that builds an annotated-jdk.jar, since I think it doesn't make sense to have a task that builds something that framework is independent of in this repo. Maybe the next step is to modify the annotated-jdk repo to have this function? I am not sure how difficult it is for a user to generate an annotated-jdk.jar file. Thank you for reviewing. |
|
Ah I see, I thought the external jdk is specified when compiling the CF. The copy is not necessary if you specify it when running the checker javac. Adding a task to generate annotated-jdk.jar sounds good to me. Maybe it's also good to look at if this improvement can also be mentioned in manual? Like chapter 35 here or chapter 36.10 here. |
Hi, I just found that there is a script for creating .jar files in the jdk repo - https://github.com/eisop/jdk/blob/master/createjdkJAR.sh. Could you confirm this file does what I think it does? I will add some descriptions to the manual. @byd110 , let me know if there is something else you'd like to change in this PR. |
… to allow the checker to only look at the annotated-jdk.jar at the same directory of the checker.jar
…and run annotatedJdkJar task when compiling
This is a hacky initial design. I have some questions about the next steps.
Currently, we can run a Gradle task that generates a JAR file for the annotated-jdk. I’ve hardcoded a file path in the checker so that it uses the JAR file I generated.
If I remove this line and run assembleForJavac, it generates a checker.jar that doesn’t contain the annotated-jdk classes. However:
The goal of this project is to provide flexibility for users, so it doesn’t really make sense to use a task in this repo to produce the JAR file. Maybe we should add a task to the annotated-jdk repo instead, and allow users to pass the path to their customized annotated-jdk as a command-line argument to the checker.
Let me know your thoughts — thanks for reviewing!