Modified combinePaths to work like Path.Combine to fix fsharp/FAKE#670 #692
Conversation
The original combinePaths insisted on stripping leading characters off of the second path, causing it to not behave like Path.Combine. /a/b/c joined with /d/e/f is supposed to come out as /d/e/f, but since the / was stripped on before we got /a/b/c/d/e/f, which caused certain operations to fail.
| @@ -29,7 +29,7 @@ let getActiveReleases dir = !!(dir @@ deploymentRootDir @@ "**/active/*.nupkg") | |||
|
|
|||
| /// Retrieves the NuSpec information for the active release of the given app. | |||
| let getActiveReleaseFor dir (app : string) = | |||
| !!(dir @@ deploymentRootDir @@ app @@ "/active/*.nupkg") | |||
| !!(dir @@ deploymentRootDir @@ app @@ "active/*.nupkg") | |||
There was a problem hiding this comment.
Without that change several of the DeploymentHelper tests failed. With the new combinePath using just a straight call to Path.Combine, app @@ "/active/.nupkg" just returns "/active/.nupkg". And the tests will fail as there isn't anything on the filesystem at /active/whatever. The leading forward slash implies its a root directory, and Path.Combine says if path2 is a root, just return path2.
In effect, saying Path.Combine("something", "/active/_.nupkg") is the same as saying Path.Combine("something", "C:\active_.nupkg") and right or not, that is supposed to return C:\active*.nupkg. There is no need to add the separator character to the beginning of path2 (the whole point of Path.Combine is to add that for you), and on linux and very likely osx, its also true that that separator can also be interpreted as a root directory indicator. Its true that its a bit confusing when thinking of @@ as +, but the original stripping of the leading / caused things to work differently on windows and linux.
The original issue I raised was presented as a question because I was unsure about fixing it as I expected that such a change as this would break more than it did. I'm guessing most everyone uses @@ as if it were a + operator, and don't fully realize its a Path.Combine under the hood and what that means, but it did cause different behavior cross platforms.
There was a problem hiding this comment.
mhm yeah. I assume this is a correct fix, but will break tons of stuff.
There was a problem hiding this comment.
Well, it didn't break any of the unit tests, but I certainly admit it is a big change as its used quite a lot and will possibly break scripts in the wild. The issue originally raised is still present though. Passing in a full path as path2 on windows just returns the full path, doing so on Linux results in a really long nested wrong path. It is of course your decision to make. Accepting the change would at least require some important wording in the next release notes. On windows, it shouldn't make any difference, but it might break some things on linux that are working around the current behavior, or that expected this behavior.
There was a problem hiding this comment.
how about introducing the (/) operator with correct behaviour and slowly making the @@ obsolete?
There was a problem hiding this comment.
That's a comment isn't it?
There was a problem hiding this comment.
damn it. I remember I saw an operator somewhere which did the same as @@. I'd like to use the same. But can't remember where.
There was a problem hiding this comment.
I thought I saw one in a previous issue thread, but I couldn't find it either. From when I was first searching for why @@ behaved the way it did.
I was thinking @@+ as it implies its like the old @@ but with more, but its ugly. Or something more like ++ but that smells of plain concatenation. <+> smells of haskell monad combinators but isn't bad. :: isn't overrideable. Nor is anything \ related. |+ might work. In any event, ive got it set to @@+ in my code, but if you come up with something better, let me know and I'll swap it out. I'm still working on a unit test that takes into consideration the different behaviors of Path.Combine under different platforms.
There was a problem hiding this comment.
There was a problem hiding this comment.
there strange implementations of (/) and also (</>)
The original combinePaths insisted on stripping leading characters off of the second path, causing it to not behave like Path.Combine. /a/b/c joined with /d/e/f is supposed to come out as /d/e/f, but since the / was stripped on before we got /a/b/c/d/e/f, which caused certain operations to fail. Since on windows roots like C:\ weren't being stripped, on windows the combinePaths worked like Path.Combine. But on Linux it would result in different results. This should cause combinePaths and @@ to work the same on both systems.
This only broke some deployment helper methods that used leading /'s, but those have been fixed. Tests pass on Windows and Linux. Just make sure not to use leading /'s in second paths if you expect those to join up like they used to.