commercialhaskell / path

Typed filepath
Other
122 stars 45 forks source link

Path.WIndows/Posix now have consistent behaviour regardless of platform #167

Closed Martinsos closed 3 years ago

Martinsos commented 3 years ago

@NorfairKing @sjakobi I started this PR as a fix for issue #166 , but I soon realized the problem is bigger than we thought (there was more bugs -> parsing, toFilePath, ...), so I tried to solve it completely.

The thing is, while Path is split into Path.Windows and Path.Posix, which use System.FilePath.[Windows|Posix], with goal of making them independent of the platform they are running on, Path.Internal was still using System.FilePath, making it dependent on the platform it was running on, and since Path.Windows and Path.Posix use Path.Internal, they were also still dependent on the platform in some parts, resulting in unexpected behavior when running Path.Windows on Linux (and possibly when running Path.Posix on Windows, I didn't get opportunity to run those tests at that point).

Reason why this was not detected so far was that tests were running Path.Windows tests when underlying OS/platform was Windows, and Posix tests when underlying platform was Mac/Linux, meaning that Path.Windows running on Posix and Path.Posix running on Windows was never tested. Therefore, the first thing I did was made both tests, for Path.Windows and Path.Posix, always run, on all platforms.
I also slightly modified tests in test/Common.hs so they don't crash when parsing fails, which was causing all the tests after them to not ever run.
At this point, when running tests on Linux, 84 tests were failing (72 from Common.hs), all from test/Windows.hs.

Next, I split Internal into Internal.Posix and Internal.Windows, which therefore made Path.Posix and Path.Windows truly independent of the platform they are running on, since now they could import specific Internal module that they need.
This also resulted with type Path not being the same type any more for Path.Windows and Path.Posix. This means they can't be mixed, which I think is even better! This was already true for Rel and Dir anyway, so I don't think it changes much in practice actually.

Finally, I realized that test/Common.hs is also platform dependent but is used in test/Windows.hs and test/Posix.hs directly, and was actually testing Path, not Path.Windows or Path.Posix. Therefore, I also split it into Common.Windows and Common.Posix via "include" mechanism.

In accordance with reasoning above, I kept changes organized in three commits, but I can squash them into one commit if needed.

So far I run tests on Linux and now they are all passing, I haven't run them for Win/Mac but expected the CI to take care of that. However, I see now that build on AppVeyor failed while setting up stack: https://ci.appveyor.com/project/chrisdone/path/builds/35817303, and this seems to have been happening for some time. Therefore, I added one more commit to fix this, and it passed (https://ci.appveyor.com/project/chrisdone/path/builds/35817924)!
If needed, we can make that commit a separate PR, or keep it as part of this PR.

Martinsos commented 3 years ago

@NorfairKing it has been some time since I created this PR, so I just wanted to check if there is anything I can do to push it forward? I am using Path in my library https://github.com/wasp-lang/strong-path and right now I am patching stuff to cover for the bugs in Path that I fixed with this PR - I would love it if instead this PR got merged and I can just use Path directly. I feel the whole PR is reasonable and sound and introduces no trade-off, just improvements, and you also validated it looks good, so what would be the next step?

NorfairKing commented 3 years ago

@Martinsos I only just realised that this was blocked on me. thanks for pinging. I'll have a look today!

Martinsos commented 3 years ago

Ah damn if I knew it was that simple I would have pinged you before, I thought we are waiting for somebody else :D! Ok great :).

NorfairKing commented 3 years ago

@Martinsos This looks entirely acceptable to me now! Merging! For the future: if you ever need something from me, you're welcome to ping me every two days :)

amesgen commented 3 years ago

src/Path/Internal/Include.hs is not mentioned path.cabal, and is therefore not part in the sdist tarball (which means that it can't be built):

 $ cabal sdist
 $ tar -ztf dist-newstyle/sdist/path-0.8.0.tar.gz | rg src
path-0.8.0/src/
path-0.8.0/src/Path.hs
path-0.8.0/src/Path/
path-0.8.0/src/Path/Include.hs
path-0.8.0/src/Path/Internal.hs
path-0.8.0/src/Path/Internal/
path-0.8.0/src/Path/Internal/Posix.hs
path-0.8.0/src/Path/Internal/Windows.hs
path-0.8.0/src/Path/Posix.hs
path-0.8.0/src/Path/Windows.hs

Solution:

diff --git a/path.cabal b/path.cabal
index b7d4a52..e7c2882 100644
--- a/path.cabal
+++ b/path.cabal
@@ -14,6 +14,7 @@ tested-with:         GHC==8.6.5, GHC==8.8.3, GHC==8.10.1
 extra-source-files:  README.md
                    , CHANGELOG
                    , src/Path/Include.hs
+                   , src/Path/Internal/Include.hs

 flag dev
   description:        Turn on development settings.

Can create a (one line) PR if desired.

NorfairKing commented 3 years ago

@amesgen I did this manually, and also did the same thing with the test include.