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

Add linktolatest option for RollingFileWriter #150

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

pabl0rg
Copy link
Contributor

@pabl0rg pabl0rg commented May 31, 2020

see #149

I'm not sure how to implement the assertions in the test. The build is not working on my machine (javac 11.0.7)

tinylog/tinylog-api/src/main/java/org/tinylog/runtime/LegacyJavaRuntime.java:[58,43] cannot find symbol
[ERROR]   symbol:   class Reflection
[ERROR]   location: package sun.reflect

If you find the idea acceptable, could you please help me with this part? Thank you!

@pmwmedia
Copy link
Member

Thank you very much for your pull request :) You need Java 9 to compile all tinylog artifacts. Java 11 and later don't include sun.reflect.Reflection anymore in favor to the new stack walker API. Java 9 and 10 are the only Java versions that can compile the legacy reflection API as well as the new stack walker API. tinylog supports both and decides on runtime which to use.

Is there any way to implement it without using Files.createSymbolicLink()? The problem is that Android added this class with API level 26, but some developers still have to support older Android versions.

@pabl0rg pabl0rg force-pushed the add-link-to-latest branch 6 times, most recently from 37ad1a8 to 5e62235 Compare June 1, 2020 03:11
@codecov
Copy link

codecov bot commented Jun 1, 2020

Codecov Report

Merging #150 into v2.2 will decrease coverage by 0.08%.
The diff coverage is 77.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##               v2.2     #150      +/-   ##
============================================
- Coverage     94.17%   94.08%   -0.09%     
- Complexity     2275     2278       +3     
============================================
  Files           115      115              
  Lines          4220     4242      +22     
  Branches        447      451       +4     
============================================
+ Hits           3974     3991      +17     
- Misses          136      139       +3     
- Partials        110      112       +2     
Impacted Files Coverage Δ Complexity Δ
...in/java/org/tinylog/writers/RollingFileWriter.java 87.12% <77.41%> (-4.88%) 30.00 <4.00> (+5.00) ⬇️
...in/java/org/tinylog/runtime/ModernJavaRuntime.java 83.87% <0.00%> (-0.51%) 9.00% <0.00%> (-1.00%)
...main/java/org/tinylog/runtime/RuntimeProvider.java 90.00% <0.00%> (-0.25%) 23.00% <0.00%> (-1.00%)
...in/java/org/tinylog/runtime/LegacyJavaRuntime.java 67.30% <0.00%> (+1.26%) 16.00% <0.00%> (ø%)
.../main/java/org/tinylog/runtime/AndroidRuntime.java 89.65% <0.00%> (+1.51%) 23.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6443e7...a1bb571. Read the comment docs.

@pabl0rg
Copy link
Contributor Author

pabl0rg commented Jun 1, 2020

It wasn't as easy as i expected to find a jdk 9 installer and to remember how to do things in java 1.6 , but it finally builds. Please let me know what you think.

@pmwmedia
Copy link
Member

pmwmedia commented Jun 1, 2020

Today, I read about creating symbolic links in Java without using the NIO API. Unfortunately, there is really no native Java API for it prior Java 7. Creating a process for executing OS commands as you did is the only way. However, the disadvantage is that these commands are OS depending. For example, it won't work on Windows.

However, I think your feature is very useful for computer applications, but I don't see any use case for Android apps. Therefore, I prefer to use your previous approach with the Files class from the NIO API. You could check if tinylog is running on Android and log an error message. Otherwise, you can execute your code. I have just added a method to detect whether running on Android: RuntimeProvider.isAndroid()

@pmwmedia
Copy link
Member

pmwmedia commented Jun 1, 2020

By the way, I have a question about removeLinkToLatest(): Why do you delete the symbolic link?

@pabl0rg pabl0rg force-pushed the add-link-to-latest branch from 5e62235 to 40d8d85 Compare June 14, 2020 14:10
@pabl0rg
Copy link
Contributor Author

pabl0rg commented Jun 14, 2020

I renamed removeLinkToLatest to filterOutSymlinks. It exists in case the user puts the symlink in the same folder as the actual log files. The logic seems much easier to read now with the java 7 sdk.

However, I am having trouble getting the build to succeed (it seems to exclude java 7 sdk):

[ERROR] /Users/juanliska/Documents/startrack/code/tinylog/tinylog-impl/src/main/java/org/tinylog/writers/RollingFileWriter.java:129: Undefined reference: java.nio.file.Path
[ERROR] /Users/juanliska/Documents/startrack/code/tinylog/tinylog-impl/src/main/java/org/tinylog/writers/RollingFileWriter.java:129: Undefined reference: java.nio.file.Path java.nio.file.Files.createSymbolicLink(java.nio.file.Path, java.nio.file.Path, java.nio.file.attribute.FileAttribute[])

Copy link
Member

@pmwmedia pmwmedia left a comment

Choose a reason for hiding this comment

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

Thank you very much for your awesome pull request! I have just reviewed it. I have only a few minor change requests. Could you apply them? Afterwards I'm going to merge your pull request.

@pabl0rg pabl0rg force-pushed the add-link-to-latest branch from 7fe6cd2 to 2911616 Compare June 15, 2020 14:09
@pabl0rg
Copy link
Contributor Author

pabl0rg commented Jun 15, 2020

Thanks for your useful comments! I made the suggested changes.

@pmwmedia
Copy link
Member

@pabl0rg Can you reproduce the failed JUnit test on your local machine?

@pabl0rg
Copy link
Contributor Author

pabl0rg commented Jun 18, 2020

Actually it seems that test case was not actually needed/wanted. (symlinks should only updated on rollover)

@pmwmedia pmwmedia merged commit a4556eb into tinylog-org:v2.2 Jun 23, 2020
@pmwmedia
Copy link
Member

@pabl0rg I have merged your pull request! Thank you very much for your contribution :)

I just noticed that you used an email address (jp@s***.info) for your commits that isn't linked to your GitHub profile. Could you add it (https://github.com/settings/emails)?

@pabl0rg
Copy link
Contributor Author

pabl0rg commented Jun 24, 2020

Done! Thank you and sorry I didn't fix the tests. I came down with covid and my thinking has been foggy.

@pmwmedia
Copy link
Member

@pabl0rg Thank you very much! Health has first priority of course. I hope you feel already better. Get well soon!

@github-actions
Copy link

github-actions bot commented Oct 9, 2022

This closed pull request has been locked automatically. However, please feel free to file an issue or create a new pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants