search

sghpjuikit / player

Audio player and management application.
22 stars 2 forks source link

Fix linkJdk task #164

Closed xeruf closed 5 years ago

xeruf commented 5 years ago

I am currently not able to build the project due to https://github.com/gradle/gradle/issues/9985, so this PR provides a fix. Furthermore, I think it does not make sense to scan the location as an output directory, because it is not of interest to this task.

sghpjuikit commented 5 years ago

Got stuck on this yesterday. I'll merge tonight.

Btw I also got stuck on kotlinc task. Didnt figure it out yet. It is severe as it can not be worked around and prevents build.

sghpjuikit commented 5 years ago

Id appreciate it if you wrote what the actual problem with the linkJdk task was, because I do not know if your solution can fix the issue nor test it. (I must have been blind)

I did confirm that changing the linkLocation will not run the task the way it was before, so I suggest adding the Input annotation.

Apart from that your handling of the location property seems to me to be wrong. Can you explain it?

xeruf commented 5 years ago
  1. Custom onlyIf will now check if there is already a link pointing to the right target
  2. I have changed gradle-wrapper.properties to use all instead of bin, so that we can see the sources in IDEA
  3. I have removed the jdkVersion property, because we don't care whether the java version changes, we only care if the java.home has moved.
xeruf commented 5 years ago

I'd say ready to merge? I would suggest a regular merge, but you could also squash.

sghpjuikit commented 5 years ago

1 The onlyIf seems much better - easy to understand and predict too 2 ok

sghpjuikit commented 5 years ago

@Xerus2000

1 Unfortunately your linkJdk upToDate check is broken. It does not handle FileNotFoundException. Caused me some pain just now. I noticed you only catch certain exceptions, but I was too busy to comment on it. Was a bug in the end.

2 Also, the idea to use log.info() instead of println() is terrible. If linkJDK task succeeds, I do not know what JDK was actually linked! That is very impractical. I do not agree that if all gradle tasks succeed, there should be no output. Why do you want the build to be black box and force developer to investigate afterwards what actually happened and whether his settings were applied? Gradle is not running with --info by default )an doing so does not appear to be a general practice) so log.info() is useless as far as I can see.

3 And exceptions are also not logged. When linkJdk check failed due to the above bug, I got:* What went wrong: Could not evaluate onlyIf predicate for task ':linkJdk'. So much information at once! Why do I need to run --debug when the error information is already there. When it fails, give me the problem asap! So I added to the catch that rethrows: logger.error("Failed linkJdk task up to date check: $e") which provides all the necessary info.

xeruf commented 5 years ago

1) Oh, that was an oversight ^^

2) Gradle tries to stay really concise. Most people don't need some output to know which JDK was linked, it just happens and works because they probably only have one. If you want it differently, I think you can change the default gradle parameters in IDEA settings, but we should adhere to the standards.

3) As for exceptions: It only gives brief information if something goes wrong. If you are anticipating an error, just run it with --stacktrace, you could even set that as default as well, that will give you the full exception.

sghpjuikit commented 5 years ago

Yeah I too dont wish to go against conventions, it just seems bizzare to me that even when the error happens, it is not logged/displayed and gradle wants me to do a 2nd run.