tecosaur / DataToolkit.jl

Reproducible, flexible, and convenient data management
https://tecosaur.github.io/DataToolkit.jl
78 stars 4 forks source link

Storage plugin file errors on Windows #28

Closed tecosaur closed 4 months ago

tecosaur commented 11 months ago

It seems that on windows, an error may be thrown during precompilation at

https://github.com/tecosaur/DataToolkitCommon.jl/blob/8749570b858844a633d7f21242b796ffe8ebf43b/src/store/inventory.jl#L223

The issue seems to be that somehow (mysteriously) the cache folder is already created as a file not a folder, leading to a mkdir error.

I have absolutely no idea what's going on with this ATM.

vtfanta commented 4 months ago

I also had this error and posted on the Discourse.

I tried tinkering around for a bit and found the following:

Temporary solution Just create the C:\\Users\\USERNAME\\AppData\\Local\\cache\\julia\\DataToolkit\\cache\\ folder yourself (you can leave it empty). This worked for me; the compilation proceeded without problems and the Inventory.toml file was created in the manually created ...\\cache folder.

tecosaur commented 4 months ago

Thanks for looking into this!

That behaviour is interesting, I thought mkpath always created a directory, not a file :confused:.

chris-b1 commented 4 months ago

I don't think it's this mkpath itself that's the issue. If I run the steps @vtfanta outlined above, with an empty C:\Users\USERNAME\AppData\Local\cache\julia\DataToolkit then cache gets created as a folder as expected. So presumably something earlier in the precompilation process is touching that dir?

vtfanta commented 4 months ago

Heh, not for me unfortunately:( With the empty DataToolkit I, again, get the chache 'file' and the error.

chris-b1 commented 4 months ago

Hmm, interesting. I should have noted originally, I do get the same precompilation error, but if I manually run those steps mkpath makes a path, not a file.

vtfanta commented 4 months ago

So presumably something earlier in the precompilation process is touching that dir?

I think you are correct. When I commented out the initially suspicious lines 232-240 in inventory.jl the cache file still got created. Now after commenting out the BaseDirs.User.cache(BaseDirs.Project("DataToolkit"), create=true) in Store.jl it no longer happens, so I think that nails it down to this line.

tecosaur commented 4 months ago

Thanks for narrowing it down guys! The BaseDirs code may be slightly hard to navigate at a glance, the relevant bit is this: https://github.com/tecosaur/BaseDirs.jl/blob/d6c8e0d9bd5910395890d8a18b4fb1242d91aebe/src/internals.jl#L50-L67

So in order to create a directory, BaseDirs.projectpath(BaseDirs.Project("DataToolkit")) needs to end with a /.

On my Linux machine, I see this:

julia> BaseDirs.projectpath(BaseDirs.Project("DataToolkit"))
"julia/datatoolkit/"

The code for Windows implementation of projectpath is here: https://github.com/tecosaur/BaseDirs.jl/blob/d6c8e0d9bd5910395890d8a18b4fb1242d91aebe/src/nt.jl#L277-L289

So from that, it looks like the fix would be adjusting how BaseDirs behaves on Windows appropriately.

tecosaur commented 4 months ago

The use of a hardcoded / instead of something platform-adaptive is so that it can be used in strings and be consistent across all platforms, e.g. BaseDirs.User.cache(BaseDirs.Project("DataToolkit"), "subfolder/", create=true).

I think what needs to be changed here might just be the behaviour when only a project is provided.

tecosaur commented 4 months ago

I've just pushed https://github.com/tecosaur/BaseDirs.jl/commit/729a9e33f23, if you might be able to try using (]add BaseDirs#729a9e33f) and seeing if it works now, that would be much appreciated!

vtfanta commented 4 months ago

Brilliant! This works on my Win10 :)

tecosaur commented 4 months ago

I'll see if I've thought of any issues with the approach in the next ~day, and if not tag that as BaseDirs v1.2.2, and close this issue.

Thanks for the help!

tecosaur commented 4 months ago

v1.2.2 is now being registered (https://github.com/JuliaRegistries/General/pull/100782), so I think I can (finally) close this :partying_face: