Skip to content

Conversation

@robstewart57
Copy link
Contributor

… Stack.Setup

The System.IO.Temp withSystemTempDirectory function from
System.IO.Temp used in Stack.Setup returns a non-canonicalised
path. However, the parseAbsDir function in the Path module of the path
library expects canonicalised paths.

The change is applied to stack setup, sanityCheck and stack upgrade
in Stack.Setup .

Fixes #1017

… Stack.Setup

The System.IO.Temp withSystemTempDirectory function from
System.IO.Temp used in Stack.Setup returns a non-canonicalised
path. However, the parseAbsDir function in the Path module of the path
library expects canonicalised paths.

The change is applied to stack setup, `sanityCheck` and stack upgrade
in Stack.Setup .

Fixes commercialhaskell#1017
@robstewart57
Copy link
Contributor Author

Hang off accepting this PR actually, this same problem is causing a problem elsewhere. More anon...

… Stack.Build.Execute

Relates to 7be7843 , which fixes the
same instances of this pattern, in Stack.Setup .

This does appear to fix commercialhaskell#1017 .
@snoyberg
Copy link
Contributor

Looks good. One thought: perhaps instead of modifying each place in the code, we could add a withSystemTempDir function to Path.IO that would handle the canonicalizing and parseAbsDir call, to make sure that future temp directory usages don't hit this.

By any change, do you have a temporary directory environment variable set to a relative path?

@robstewart57
Copy link
Contributor Author

@snoyberg thanks for the review, and I agree. Rather than moving a version of withSystemTempDirectory to the path library, I've submitted a pull request to the temporary library.

batterseapower/temporary#16

If this gets accepted to temporary, I have a corresponding stack commit that uses the withCanonicalisedSystemTempDirectory function throughout, in place of withSystemTempDirectory. And I will submit another stack PR with this commit if or when the https://github.com/robstewart57/temporary/commit/6db0f9952d66d74363cb127d00b7586aefe3c53e temporary commit gets reflected on hackage.

So I'm closing this PR.

@snoyberg
Copy link
Contributor

Actually, Path.IO is in stack itself, so it could just be added here. I'd still recommend going that route, as it will be easier to deal with dependencies that way.

@robstewart57
Copy link
Contributor Author

@snoyberg ah ha! I've got to the bottom of this bug. It's a really interesting corner case.

The withSystemTempDirectory is used in a number of places throughout stack. It's implementation in the temporary library uses getTemporaryDirectory from System.Directory. The documentation says:

On Unix, getTemporaryDirectory returns the value of the TMPDIR environment variable or "/tmp" if the variable isn't defined.

I've looked into my ~/.bash_profile and sure enough,

# for bibtex2html
export TMPDIR=.

The bib2texhtml documentation says to do this:

Note for users using a version of bibtex2html prior to 1.98: TeXlive 2010 prevents bibtex2html to run bibtex in a temporary directory, resulting in an error message such as .... A workaround consists in telling bibtex2html to use the current directory for temporary files, using the following shell command before running bibtex2html:

export TMPDIR=.

The path starting "." is eventually passed back to stack in all places where withSystemTempDirectory is used in stack. The stack code base always assumes a canonicalised path is provided from this function, though as this TMPDIR=. corner case demonstrates that this is not necessarily the case.

@snoyberg What do think is the best resolution?

  1. Ignore this corner case, the number of users who've specified the TMPDIR environment variable, for bibtex2html or otherwise, is tiny.
  2. Canonicalise the path received from withSystemTempDirectory, as per my first PR.
  3. Copy the temporary library functions to Path.IO in stack, with each modified to ensure that all with* functions return canonicalised paths.

@snoyberg
Copy link
Contributor

I'd still say 3 is the right approach. Just to be clear though, I don't
anticipate any code being copied from the temporary package, those library
functions can just be wrapped.

On Wed, Sep 23, 2015, 12:01 AM Rob Stewart [email protected] wrote:

@snoyberg https://github.com/snoyberg ah ha! I've got to the bottom of
this bug. It's a really interesting corner case.

The withSystemTempDirectory is used in a number of places throughout
stack. It's implementation
https://github.com/batterseapower/temporary/blob/master/System/IO/Temp.hs#L35
in the temporary library uses getTemporaryDirectory from
System.Directory. The documentation
https://hackage.haskell.org/package/directory-1.2.3.1/docs/System-Directory.html#v:getTemporaryDirectory
says:

On Unix, getTemporaryDirectory returns the value of the TMPDIR environment
variable or "/tmp" if the variable isn't defined.

I've looked into my ~/.bash_profile and sure enough,

for bibtex2html

export TMPDIR=.

The bib2texhtml documentation
https://www.lri.fr/%7Efilliatr/bibtex2html/ says to do this:

Note for users using a version of bibtex2html prior to 1.98: TeXlive 2010
prevents bibtex2html to run bibtex in a temporary directory, resulting in
an error message such as .... A workaround consists in telling bibtex2html
to use the current directory for temporary files, using the following shell
command before running bibtex2html:

export TMPDIR=.

The path starting "." is eventually passed back to stack in all places
where withSystemTempDirectory is used in stack. The stack code base
always assumes a canonicalised path is provided from this function, though
as this TMPDIR=. corner case demonstrates that this is not necessarily
the case.

@snoyberg https://github.com/snoyberg What do think is the best
resolution?

Ignore this corner case, the number of users who've specified the
TMPDIR environment variable, for bibtex2html or otherwise, is tiny.
2.

Canonicalise the path received from withSystemTempDirectory, as per my
first PR.
3.

Copy the temporary library functions to Path.IO in stack, with each
modified to ensure that all with* functions return canonicalised paths.


Reply to this email directly or view it on GitHub
#1019 (comment)
.

robstewart57 added a commit to robstewart57/stack that referenced this pull request Sep 24, 2015
When the $TMPDIR environment variable is set, the directory paths
provided by `withSystemTempDirectory` and `withTempDirectory` from
System.IO.Temp provided by the temporary library are not
canonicalised. This commit wraps these functions into canonicalized
versions.

See an earlier PR for discussion commercialhaskell#1019

Fixes commercialhaskell#1017
@robstewart57
Copy link
Contributor Author

Done #1047 .

robstewart57 added a commit to robstewart57/stack that referenced this pull request Sep 24, 2015
When the $TMPDIR environment variable is set, the directory paths
provided by `withSystemTempDirectory` and `withTempDirectory` from
System.IO.Temp provided by the temporary library are not
canonicalised. This commit wraps these functions into canonicalized
versions.

See an earlier PR for discussion commercialhaskell#1019

Fixes commercialhaskell#1017
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