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.

Sign up for GitHub

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

Modified combinePaths to work like Path.Combine to fix fsharp/FAKE#670 #692

Closed
wants to merge 1 commit into from
Closed

Modified combinePaths to work like Path.Combine to fix fsharp/FAKE#670 #692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/app/Fake.Deploy/DeploymentHelper.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mhm yeah. I assume this is a correct fix, but will break tons of stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about introducing the (/) operator with correct behaviour and slowly making the @@ obsolete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a comment isn't it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mausch found it: http://stackoverflow.com/questions/2812084/overload-operator-in-f

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there strange implementations of (/) and also (</>)

|> Seq.map GetMetaDataFromPackageFile
|> Seq.head

Expand All @@ -38,7 +38,7 @@ let getAllReleases dir = !!(dir @@ deploymentRootDir @@ "**/*.nupkg") |> Seq.map

/// Retrieves the NuSpec information for all releases of the given app.
let getAllReleasesFor dir (app : string) =
!!(dir @@ deploymentRootDir @@ app @@ "/**/*.nupkg") |> Seq.map GetMetaDataFromPackageFile
!!(dir @@ deploymentRootDir @@ app @@ "**/*.nupkg") |> Seq.map GetMetaDataFromPackageFile

/// Returns statistics about the machine environment.
let getStatistics() = getMachineEnvironment()
Expand Down Expand Up @@ -102,7 +102,7 @@ let runDeploymentFromPackageFile workDir packageFileName scriptArgs =
/// Rolls the given app back to the specified version
let rollback workDir (app : string) (version : string) =
try
let currentPackageFileName = !!(workDir @@ deploymentRootDir @@ app @@ "/active/*.nupkg") |> Seq.head
let currentPackageFileName = !!(workDir @@ deploymentRootDir @@ app @@ "active/*.nupkg") |> Seq.head
let backupPackageFileName = getBackupFor workDir app version
if currentPackageFileName = backupPackageFileName then
Failure { Messages = Seq.empty
Expand Down Expand Up @@ -138,10 +138,10 @@ let getVersionFromNugetFileName (app : string) (fileName : string) =
/// Returns the version no. of the latest backup of the given app
let getPreviousPackageVersionFromBackup dir app versions =
let currentPackageFileName =
!!(dir @@ deploymentRootDir @@ app @@ "/active/*.nupkg")
!!(dir @@ deploymentRootDir @@ app @@ "active/*.nupkg")
|> Seq.head
|> getVersionFromNugetFileName app
!!(dir @@ deploymentRootDir @@ app @@ "/backups/*.nupkg")
!!(dir @@ deploymentRootDir @@ app @@ "backups/*.nupkg")
|> Seq.map (getVersionFromNugetFileName app)
|> Seq.filter (fun x -> x < currentPackageFileName)
|> Seq.toList
Expand All @@ -166,4 +166,4 @@ let rollbackTo workDir app versionInfo =
Message = sprintf "Rollback to version (%s-%s) failed" app versionInfo
Timestamp = DateTimeOffset.UtcNow } ]
IsError = true
Exception = e }
Exception = e }
2 changes: 1 addition & 1 deletion src/app/FakeLib/EnvironmentHelper.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ type EnvironTarget = EnvironmentVariableTarget
let environVar name = Environment.GetEnvironmentVariable name

/// Combines two path strings using Path.Combine
let inline combinePaths path1 (path2 : string) = Path.Combine(path1, path2.TrimStart [| '\\'; '/' |])
let inline combinePaths path1 path2 = Path.Combine(path1, path2)

/// Combines two path strings using Path.Combine
let inline (@@) path1 path2 = combinePaths path1 path2
Expand Down
30 changes: 30 additions & 0 deletions src/test/Test.FAKECore/EnvironmentHelperSpecs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
using System.Linq;
using Fake;
using Machine.Specifications;

namespace Test.FAKECore
{
public class when_combining_paths
{
It should_act_as_path_combine =
() => {
var testValues = new []{
new []{"c:\\test", "dir\\asdf"},
new []{"\\test", "dir\\asdf"},
new []{"test", "dir\\asdf"},
new []{"/test", "dir/asdf"},
new []{"/test", "/dir/asdf"},
new []{"c:\\test", "d:\\dir\\asdf"},
new []{"/asdf/asdf/asdf/", "/asdf/asdf"},
new []{"c:\\test", "\\\\dir\\asdf"},
new []{"c:\\test", "/dir\\asdf"},
};

foreach(var item in testValues)
{
EnvironmentHelper.combinePaths(item[0], item[1])
.ShouldEqual(System.IO.Path.Combine(item[0], item[1]));
}
};
}
}
3 changes: 2 additions & 1 deletion src/test/Test.FAKECore/Test.FAKECore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@
<Compile Include="SideBySideSpecification\ReferencesScanningSpecs.cs" />
<Compile Include="SideBySideSpecification\SpecsRemovementSpecs.cs" />
<Compile Include="StringHelperSpecs.cs" />
<Compile Include="EnvironmentHelperSpecs.cs" />
<Compile Include="MessageFiles\MessageFileSpecs.cs" />
<Compile Include="ConfigFiles\ConfigSpecs.cs" />
<Compile Include="AssemblyInfoSpecs.cs" />
Expand Down Expand Up @@ -556,4 +557,4 @@
</ItemGroup>
</When>
</Choose>
</Project>
</Project>