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

Fix #992 test failure when space in temp path. #1076

Merged
merged 2 commits into from
Jan 17, 2016

Conversation

bentayloruk
Copy link
Contributor

Tests invoking csc.exe were failing when paths contained a space. Updated csc invocation to ensure quote references, output path and input file paths when a space is detected and not already quoted.

Tried to be light touch and backwards compatible. Not sure how widely used CscHelper is used in the wild.

// Sensitive to being backwards compatible with people that are using
// Csc AND quoting their paths. Only quote if space in path and quotes not detected.
// MAYBE this should go in the FileSystemHelper module?
if path.Contains(" ") then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking this should probably be trimmed. Currently we could double quote a path that is already quoted, but book ended with whitespace.

Tests invoking csc.exe were failing when paths contained a
space.  Updated csc invocation to ensure quote references, output path and
input file paths when a space is detected and not already quoted.
Don't think anyone would intentionally pad a path and doubt Csc would care if they did, so trim paths.  This change avoids us quoting a path that is quoted, but with whitespace at one end, the other or both.
@bentayloruk bentayloruk force-pushed the fix-992-quote-csc-paths branch from db9a850 to 6f66ec7 Compare January 16, 2016 12:16
@@ -78,11 +78,22 @@ let cscExe toolPath (srcFiles : string list) (opts : string list) : int =
/// Target = ...
/// ... })
let csc (setParams : CscParams -> CscParams) (inputFiles : string list) : int =
let inputFiles = inputFiles |> Seq.toList
// Helper to quote a path with spaces in it, if not already quoted. See https://github.com/fsharp/FAKE/issues/992
let ensureTrimQuotedPath (path : string) =
Copy link
Member

Choose a reason for hiding this comment

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

Can we put this in one of the other helpers? Maybe we already have something similar

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 could not see a helper for this. My search for ""%s"" yielded a handful of other places doing this. I will add a helper function trimAndEnsureQuotedIfRequired to the FileSystemHelper.fs.

Copy link
Member

Choose a reason for hiding this comment

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

Thx.
On Jan 16, 2016 2:49 PM, "Ben Taylor" [email protected] wrote:

In src/app/FakeLib/CscHelper.fs
#1076 (comment):

@@ -78,11 +78,22 @@ let cscExe toolPath (srcFiles : string list) (opts : string list) : int =
/// Target = ...
/// ... })
let csc (setParams : CscParams -> CscParams) (inputFiles : string list) : int =

  • let inputFiles = inputFiles |> Seq.toList
  • // Helper to quote a path with spaces in it, if not already quoted. See Build error locally #992
  • let ensureTrimQuotedPath (path : string) =

I could not see a helper for this. My search for ""%s"" yielded a
handful of other places doing this. I will add a helper function
trimAndEnsureQuotedIfRequired to the FileSystemHelper.fs.


Reply to this email directly or view it on GitHub
https://github.com/fsharp/FAKE/pull/1076/files#r49931969.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm hesitant about making this a general function. Digging around, this is not as simple as it first appears. Might be better to leave it local to the csc function. It is already local in the other places.

@bentayloruk bentayloruk mentioned this pull request Jan 17, 2016
forki added a commit that referenced this pull request Jan 17, 2016
Fix #992 test failure when space in temp path.
@forki forki merged commit 492a157 into fsprojects:master Jan 17, 2016
@forki
Copy link
Member

forki commented Jan 17, 2016

Thx

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