Skip to content

Remove lazy_static + cleanup lib.rs #6

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 2 commits into from
Oct 23, 2024

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Jul 26, 2024

This PR performs general cleanup on lib.rs:

  • lazy_static is removed as a dependency
    • Instead we can just directly use #[cfg(target_os = "..")] as needed.
  • A unique implementation of do_locate_java_home is provided for windows, macos and unix.
    • This approach makes it very easy to understand the implementation for a single system, since they deviate so much it doesnt really make sense to try following the flow of multiple systems.
  • Removed a bunch of allocations by avoiding clones and &str -> String conversions
  • Fixes a bug on windows where the last path output by where was chosen.
    • The correct behavior is to choose the first path as that is the path that java will be run from.
    • I removed the warning here since its reasonable for the user to have multiple java's in their path, no point in warning about it, just pick the correct one.

As a result of the cleanup:

  • The code should be very slightly faster, its certainly not a hotpath but applications starting up slightly faster is always nice.
  • Improve compile times
    • one less dependency to build
  • The compiled binaries for java-locator are smaller.
    • -134KB for debug
    • -10KB for release
  • The implementation is more readable.
  • There is less total lines of code

future cleanup

I've left some changes out of this PR since they are breaking changes to the API so we should consider them separately.

  • Return PathBuf instead of String
    • since we are returning a path it should really be a PathBuf instead of a String.
  • replace glob with manual recursive search implementation
    • I was hoping to remove the usage of glob in this PR since we are spending ~200ms in the call to glob on my machine.
    • we expose globing to the user as part of the java-locator API, so removing glob would be a breaking change.

@rukai rukai force-pushed the cleanup branch 2 times, most recently from 7af3cc5 to 09ee779 Compare July 26, 2024 06:41
@rukai
Copy link
Contributor Author

rukai commented Oct 21, 2024

Hi, any chance to take a look at this?

Copy link
Owner

@astonbitecode astonbitecode left a comment

Choose a reason for hiding this comment

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

Apologies for not dealing with it earlier.

I am ok with most of the proposed changes.

I believe it is better though to have warning when multiple java locations are found. Explicitness is better than ignorance.

Thanks for your effort!

@rukai
Copy link
Contributor Author

rukai commented Oct 22, 2024

Thankyou for your review, I've made the appropriate changes and the PR is ready for rereview.

@astonbitecode astonbitecode merged commit dac6523 into astonbitecode:master Oct 23, 2024
3 checks passed
@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