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

Remove as much dependencies as possible #172

Merged
merged 2 commits into from
Aug 25, 2024

Conversation

longemen3000
Copy link
Contributor

work on #171

@rofinn
Copy link
Owner

rofinn commented Aug 24, 2024

Looking through the docs extensions were only added in 1.9. The proposed changes would be breaking for earlier versions. I'll work on a PR that supports both to avoid an unnecessary breaking change.

@longemen3000
Copy link
Contributor Author

Is it possible to approve tests on this branch? I looked at the Compat dependency and it is not used directly (at a first glance). At the moment only Mmap was moved to an extension, while being unconditionally loaded in Julia versions without extension support. The rest of removed packages were replaced by similar functionality (Printf is now a string concatenation, uuid4 is a 4-line function)

@rofinn
Copy link
Owner

rofinn commented Aug 24, 2024

No, I can approve, but I need to re-enable the GH actions. I'm just gonna test locally on an older Julia release. We also need to move the test.jl file into an extension. Are you fine with leaving Dates as is though? It's probably the largest dep, but it's also the most intertwined.

@longemen3000
Copy link
Contributor Author

Yeah, Dates is a necessary dependency, nothing to do there

Copy link
Owner

@rofinn rofinn left a comment

Choose a reason for hiding this comment

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

These change don't run for me on 1.6 or 1.10.

src/utils.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
@longemen3000
Copy link
Contributor Author

Btw, tests also fail on 1.11, I'm looking what is causing those failures

@longemen3000
Copy link
Contributor Author

longemen3000 commented Aug 25, 2024

Compat is necessary for julia versions < 1.6, because they add a new method for Base.include. it is not necessary to load it in new julia versions, so, while it is installed, it is not loaded if necessary.

the errors in 1.11 were because of JuliaLang/julia#53699, that defines and exports Base.isexecutable, clashing with FilePathsBase exports. following the line of similar functions, if Base.isexecutable is available, FilePathsBase just adds methods to the julia version instead of creating FilePathsBase.isexecutable

i also migrated the TestPaths module into an extension. the module and it's functions are available, but the actual methods are only loaded at extension time.

all tests seems to pass now: https://github.com/longemen3000/FilePathsBase.jl/actions/runs/10544795318

Copy link
Owner

@rofinn rofinn left a comment

Choose a reason for hiding this comment

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

Awesome work! Looks pretty good to me. Most of my feedback is git related.

  1. Did you want to cleanup the history a bit (ie: rebase and squash some of the debug commits)?
  2. Could you move the CI and gitignore changes to a separate PR since they're unrelated to this PR, AFAICT?
  3. I think using git mv for src/test would make the PR a lot cleaner, and easier to review for potentially breaking changes. Especially since src/test is basically just a stub now.
  4. I have my own formatting preferences, but at this point I'll just make a separate PR that makes those changes and adds a JuliaFormatter CI job. I suspect the Blue style guide is pretty close to what I'm already doing.

Oh, do you have an idea of how much these change improve load times where these standard libs aren't in the system image anymore?

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
ext/FilePathsBaseTestExt.jl Show resolved Hide resolved
src/FilePathsBase.jl Outdated Show resolved Hide resolved
src/system.jl Outdated Show resolved Hide resolved
src/test.jl Show resolved Hide resolved
This was referenced Aug 25, 2024
@longemen3000
Copy link
Contributor Author

on 1.11-rc2:

master:

julia> @time_imports using FilePathsBase
Precompiling FilePathsBase...
1 dependency successfully precompiled in 3 seconds. 14 already precompiled.
    0.8 ms  Compat
    41.1 ms  Test
                ┌ 0.0 ms FilePathsBase.__init__() 
    295.6 ms  FilePathsBase 40.41% compilation time

this PR:

julia> @time_imports using FilePathsBase
               ┌ 0.0 ms FilePathsBase.__init__() 
    182.7 ms  FilePathsBase 82.77% compilation time
      0.5 ms  FilePathsBase → FilePathsBaseMmapExt

seems that the overall effect is positive in loading times, this does not include the time not spent loading testing code.

@rofinn
Copy link
Owner

rofinn commented Aug 25, 2024

Awesome, thanks for all the hard work on this! Looks like a pretty clean set of changes now 👍🏻 I'll merge the other PRs and tag a release later today.

@rofinn rofinn merged commit 25bbdb9 into rofinn:master Aug 25, 2024
6 checks passed
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