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

`nim js -o:dirname main.nim` writes nothing, and no error shown #9154

Open
timotheecour opened this Issue Oct 2, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@timotheecour
Copy link
Collaborator

timotheecour commented Oct 2, 2018

mkdir dirname
nim js -o:dirname main.nim writes nothing, and no error shown

it should either write to dirname/main.js, or error with : cannot write to dirname (dirname is a directory)

@LemonBoy

This comment has been minimized.

Copy link
Collaborator

LemonBoy commented Oct 3, 2018

Some hints for the next hacktoberfest{er,ee} (is that even a word?)

  • This problem affects the frontend, namely the part that parses the option strings.
  • You don't see this problem with the C backend because gcc is smart enough to warn you that the chosen output path is not a file
  • The parsing happens in processSwitch
  • The compiler/pathutils module contains some interesting methods
  • What you want to do is to check that:
    • The given argument is a path + filename (eg. /a/b/c -> path: /a/b, filename: c)
    • The path exists

The deeper problem is inside the jsgen.nim file since it uses writeRopeIfNotEqual to write out the file and it returns false if the file contents are the same and if the file cannot be opened. I believe it's been introduced (~10 years ago, time flies!) in order to provide some kind of very late caching that's probably not needed anymore, but let's ask @Araq about this first.

Trivia fact: The js backend at some point in time was also a PHP backend and, before that, a luaJIT one!

manterolat added a commit to manterolat/Nim that referenced this issue Oct 4, 2018

Fixes nim-lang#9154
Add checks for:
 - The output file actually being a directory
 - A nonexistant path to the output file

@Araq Araq closed this in 16a941a Oct 10, 2018

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 10, 2018

@manterolat @Araq @LemonBoy could we reopen?

mkdir /tmp/d06/ && /tmp/d06/
nim js -o:/tmp/d06/bar2/ main.nim

creates bar2/ directory but no js file is created, and no error

with s/js/c/ an error is shown ld: can't open output file for writing

maybe we could throw in case dirname contains trailing DirSep ? Note: this is not contrived, and can happen in scripts, eg:
nim js -o:$dir/$name foo.nim where env var name is empty for example

@Araq

This comment has been minimized.

Copy link
Member

Araq commented Oct 11, 2018

-o doesn't take a directory.

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 11, 2018

sure, but then it should error instead of silently doing I-don't-know-what (no error is shown, and I've no idea whether a file was produced or not, nor where, if it is produced); like I've said, in more complex cases (eg in scripts), user could issue a command nim js -o:$dir/$name foo.nim where $name ends up empty, in which case it should error, but instead something mysterious happens

@manterolat

This comment has been minimized.

Copy link
Contributor

manterolat commented Oct 12, 2018

I could add a small check in commands.nim (or in cgen.nim and jsgen.nim) that raises an error if the argument is already a directory:

if dirExists(AbsoluteDir outfile):
  raise newException(IOError, "cannot create file (" & outfile.string & " is a directory")
@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 12, 2018

according to principle of catching input errors as early as possible, I think in commands.nim (ie before it reaches cgen.nim and jsgen.nim IIRC) is where the test should be

@timotheecour timotheecour changed the title `nim js -o:dirname main.nim` writes nothing, and no error shown [TODO] `nim js -o:dirname main.nim` writes nothing, and no error shown Oct 12, 2018

@LemonBoy

This comment has been minimized.

Copy link
Collaborator

LemonBoy commented Oct 12, 2018

Indeed, and if the folder does not exist it should also be created there and not in every damn backend.
Moreover the use of writeRopeIfNotEqual hides any error during the output writing so maybe that should be changed (removed?)

krux02 added a commit to krux02/Nim that referenced this issue Oct 15, 2018

@timotheecour

This comment has been minimized.

Copy link
Collaborator

timotheecour commented Oct 20, 2018

@Araq @krux02 @manterolat could we reopen?

this silently does nothing (no error, no /tmp/d16/dochack.js file generated)

ls /tmp/d16
cannot access '/tmp/d16': No such file or directory
nim js -o:/tmp/d16/dochack.js tools/dochack/dochack.nim
ls /tmp/d16
cannot access '/tmp/d16': No such file or directory

(likewise with case mentioned in #9154 (comment) with nim js -o:/tmp/d06/bar2/ main.nim which silently does nothing instead of giving an error )

@Araq Araq reopened this Oct 22, 2018

@timotheecour timotheecour changed the title [TODO] `nim js -o:dirname main.nim` writes nothing, and no error shown `nim js -o:dirname main.nim` writes nothing, and no error shown Oct 22, 2018

narimiran added a commit to narimiran/Nim that referenced this issue Oct 31, 2018

narimiran added a commit to narimiran/Nim that referenced this issue Nov 1, 2018

narimiran added a commit that referenced this issue Nov 1, 2018

narimiran added a commit that referenced this issue Nov 1, 2018

Fixes #9154 (#9193)
(cherry picked from commit 16a941a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment