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

Use standard C++ filesystem API only #1665

Merged
merged 29 commits into from
Aug 17, 2022
Merged

Conversation

Klaim
Copy link
Member

@Klaim Klaim commented May 10, 2022

See rationale in libmamba/include/mamba/core/mamba_fs.hpp

@Klaim Klaim marked this pull request as draft May 10, 2022 16:06
@Klaim Klaim force-pushed the klaim/std-filesystem branch 2 times, most recently from f80df10 to a4d8879 Compare May 11, 2022 15:06
@Klaim
Copy link
Member Author

Klaim commented May 13, 2022

Here is where I am at:

  • I fixed all conversion errors on my local build with visual studio;
  • I didn't address yet the linking issues visible on current CI build, I don't see them locally
  • For some reason I can't link a Release build because of symbols missing from fmt, I suspect it's because I'm not using the same compiler as the fmt's dll (I used VS2022 IDE instead of building from cli, so that's understandable);
  • With the debug build, I get runtime errors coming from the YAML-CPP implementation, so it is opaque, but it might be due to the same mismatch of build than for fmt;
  • I suspect that the path->string conversions will fail on runtime on Windows (with no experimental UTF-8 mode) whith unicode paths, but I didn't reach the point where I could test that yet. Hopefully the pytest mamba/tests/ should catch these. There are several solutions if it fails, we should discuss that once this at least build on all platforms.

@Klaim
Copy link
Member Author

Klaim commented May 18, 2022

This version compiles and run on my Windows which is set to have UTF-8 by default in everywhere. However only the libmamba tests can be run, for some reason the pytest based tests all fail to find some executable.

@wolfv
Copy link
Member

wolfv commented May 19, 2022

@Klaim it looks like one last Unicode test fails on mamba.

@Klaim
Copy link
Member Author

Klaim commented May 19, 2022

@Klaim it looks like one last Unicode test fails on mamba.

Yes, it's the one revealing the need for wrapping the path type. I was doing that but I'm starting to think it might be more complicated than just using a std::string to_utf8(const fs::path& path) function that would do the work where we need it.
From what I tried, wrapping the type is a bit brittle because it means every time we use a free function or operator from std::filesystem::path we end up with that type instead of fs::path and that can lead to bad surprises.
I'm trying to figure out what would be best without disturbing too much of the code.

@Klaim Klaim force-pushed the klaim/std-filesystem branch 7 times, most recently from afa0559 to 406556e Compare May 25, 2022 11:08
@Klaim Klaim mentioned this pull request Jun 2, 2022
@Klaim Klaim force-pushed the klaim/std-filesystem branch 7 times, most recently from 5e909d9 to 3582265 Compare June 17, 2022 12:53
@Klaim Klaim marked this pull request as ready for review June 17, 2022 14:56
@Klaim Klaim force-pushed the klaim/std-filesystem branch 3 times, most recently from cc99827 to c09f0e0 Compare June 29, 2022 13:49
@Klaim
Copy link
Member Author

Klaim commented Aug 12, 2022

(rebased over CI not reporting issues that should have been reported, which I expect to fail because I added a test that should fail before I will push more commits).

@Klaim
Copy link
Member Author

Klaim commented Aug 12, 2022

OK Now the test fails as it should.
I will now smash the commits for the test together and add my solution (that cannot run locally)

@Klaim
Copy link
Member Author

Klaim commented Aug 12, 2022

looks like my workaround for fs::removeand fs::remove_all when VS is old works (at least on the CI)

cleanup incoming

@wolfv wolfv merged commit e0527eb into mamba-org:master Aug 17, 2022
@wolfv
Copy link
Member

wolfv commented Aug 17, 2022

Thanks for pushing this through until the end!

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