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

Absorb vtjnash/Pidfile.jl into FileWatching #42471

Closed

Conversation

IanButterworth
Copy link
Member

I took a stab at bringing https://github.com/vtjnash/Pidfile.jl in, as discussed in JuliaLang/Pkg.jl#2732 (comment)

FileWatching seemed like a good place for it?

The primary reason is to provide a robust way for Pkg to fix multiprocess races like JuliaLang/Pkg.jl#2746 and #42405

The only changes I made were to fix imports and conform docstring formatting.

cc. @vtjnash

@IanButterworth IanButterworth requested a review from vtjnash October 2, 2021 06:14
@vtjnash
Copy link
Member

vtjnash commented Oct 2, 2021

Hm, it does make some sense being part of there. But any reason not to keep it separate (with separate issue tracker and such)? There's a script contrib/new-stdlib.sh for importing those.

@IanButterworth
Copy link
Member Author

I was thinking it was a reasonable fit and keeping the number of stdlibs low seems to be a bit of a goal? Third option, move FileWatching as it is in this PR to an external stdlib?

@DilumAluthge
Copy link
Member

Are you planning on making Pidfile.jl part of the public API of the FileWatching stdlib? If so, I would recommend making Pidfile an external stdlib instead, similar to Pkg, Downloads, etc. (Normally I wouldn't recommend adding a new stdlib, but the motivation here is to use the code from Pidfile in Pkg, and Pkg cannot have any non-stdlib dependencies.)

On the other hand, if you don't want to make it part of the public API, we could just vendor a copy of the Pidfile.jl code in the Pkg repo.

I would recommend not making it part of the public API, since we really only want to use this for Pkg. That way, in the future, we can make breaking changes (or even remove Pidfile completely) in a non-breaking release of Julia.

@vtjnash
Copy link
Member

vtjnash commented Oct 2, 2021

We also very much want to use this for many other code loading places in Base and REPL.

@DilumAluthge
Copy link
Member

We also very much want to use this for many other code loading places in Base and REPL.

In that case, I'd just make it its own external stdlib like Pkg, Downloads, etc.

Users can then use the BumpStdlibs bot (https://github.com/JuliaLang/BumpStdlibs.jl) to automate updates.

@KristofferC
Copy link
Member

In that case, I'd just make it its own external stdlib like Pkg, Downloads, etc.

If we want to use it in Base, why does that make making it an external library a good idea? In my head, it's the opposite.

@DilumAluthge
Copy link
Member

In that case, I'd just make it its own external stdlib like Pkg, Downloads, etc.

If we want to use it in Base, why does that make making it an external library a good idea? In my head, it's the opposite.

Ohh, I see what you mean. Same as with TOML, where the Base code loading needs to use it.

Okay, so instead of an external library, should we make Pidfile a "vendored" stdlib similar to TOML?

@DilumAluthge
Copy link
Member

FWIW, the BumpStdlibs.jl bot will eventually be able to update "vendored" stdlibs like TOML.

So we can still use the https://github.com/vtjnash/Pidfile.jl repo as the main development repo, and then sync the files like we do for TOML.

@KristofferC
Copy link
Member

The TOML thing didn't turn out very well though. So perhaps just keep it completely in the repo? Not sure.

@DilumAluthge
Copy link
Member

The TOML thing didn't turn out very well though.

How do you mean?

@DilumAluthge
Copy link
Member

So perhaps just keep it completely in the repo?

By this, you mean that we could move the contents of Pidfile.jl into the JuliaLang/julia repo, and then we would archive the https://github.com/vtjnash/Pidfile.jl repo? And then all development (issues, pull requests, etc.) would just happen directly in the JuliaLang/julia repo?

That seems fine to me.

@IanButterworth
Copy link
Member Author

IanButterworth commented Oct 2, 2021

If it's going to be used in Base for things like code loading then that sets the requirement that it needs to be in Base, I believe. And nothing else in Base is externally hosted, so it seems like simply putting it in Base in the main repo is the only option?

@IanButterworth
Copy link
Member Author

I'm assuming agreement so will put this directly in Base in another PR

@vtjnash
Copy link
Member

vtjnash commented Oct 8, 2021

TOML, Logging, and a few others are used in Base but appear as stdlibs. I'd also like to move Base.Filesystem to an FS stdlib.

@IanButterworth
Copy link
Member Author

Ok, I'll add it as an external stdlib. But before doing that I guess it should be moved to the JuliaLang org?

@IanButterworth
Copy link
Member Author

Replaced by #42571

@IanButterworth IanButterworth deleted the ib/add_pidfile branch October 9, 2021 19:29
@IanButterworth
Copy link
Member Author

@vtjnash now that Pidfile 1.3.0 is out, should I revive this and bring everything over? or happy to leave it to you

@vtjnash
Copy link
Member

vtjnash commented Feb 22, 2022

Yep, happy to have you bring it over. Fixed some APIs and added some features in v1.3.0 to prepare it for that.

@IanButterworth IanButterworth restored the ib/add_pidfile branch February 27, 2022 04:53
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.

4 participants