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

[RFC] 'walkDir' now has a new 'checkDir' flag, to mimic behaviour of other languages #13642

Merged
merged 8 commits into from
Mar 20, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 13, 2020

pretty much every single language I've every used (see list below in comments) raises when input dir is invalid, and so should nim.

future work

provide a means for users to report errors encountered during walkDirRec

note

Yes, I'm aware of #13163 (/cc @a-mr ) #13011 (/cc @demotomohiro ). These can still be done after this PR.

@timotheecour timotheecour changed the title walkDir, walkDirRec,listFiles now throw if input dir invalid walkDir, walkDirRec,listFiles now throw if input dir is invalid Mar 13, 2020
@a-mr
Copy link
Contributor

a-mr commented Mar 13, 2020

AFAIR when demotomohiro in #13011 tried to do exception raising as default option, he got test failures, so Araq rejected it as breaking compatibility "in our own code".

Nevertheless I fully support this. The current Nim behavior looks downright stupid to me. Compatibility basis is far-fetched since this behavior of silently omitting the open error has never been documented, hence programs relying on it are incorrect.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 14, 2020

he got test failures

also getting failures in important_packages but I think they're (almost) all originating from a single place, somewhere inside nimble I think; I need to investigate further

@timotheecour
Copy link
Member Author

timotheecour commented Mar 15, 2020

here are bugs I found thanks to this PR:

all the failures are originating from 2 lines in nimble, I'm fixing it in nim-lang/nimble#780 (EDIT: but that PR was unfortunately closed)

@juancarlospaco
Copy link
Collaborator

Does Fix #11458 (comment) ?. 🙂

@timotheecour
Copy link
Member Author

Does Fix #11458 (comment) ?. 🙂

no, but #13689 fixes it

@timotheecour
Copy link
Member Author

this PR is blocked until nim-lang/nimble#780 gets re-opened and merged /cc @dom96

@Araq
Copy link
Member

Araq commented Mar 19, 2020

Rejected, breaking change. If you care so much about existing directories you can easily check for dirExists first.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 19, 2020

Rejected, breaking change

The way I see it, the only breaking changes will actually be bug fixes in user code.

let's at least open the debate on this one and get more opinions and perspectives, I'm not the only one supporting this.

The only breaking change amongst all packages is a single line in nimble (or 2 lines depending on how you see it), which I fixed here nim-lang/nimble#780
once that PR is merged, everything passes, including all important_packages.

The current behavior is downright error-prone and source of bugs (try: for ai in walkDir(getEnv("MISSPELLED_ENV_VAR")): echo ai), eg the 3 ones I found thanks to this PR (see #13642 (comment)), where nimble test was happily reporting success even though neither nimble test task existed nor tests dir existed (ie, walkDir("tests") should've raised instead of noop)

check for dirExists first

that's a racy condition, and also doesn't work: if you don't have read access to input dir, dirExists(dir) returns true and walkDir yields nothing (and no error), which is clearly buggy. After PR you either raise (by default) or yield nothing if checkDir=false. And yes, I took care of walkDirRec to have sane behavior in the recursive case.

other languages also raise for invalid dir: python, D, C, C++, nodejs, matlab/octave, ruby, swift, shell(ls, find)

  • python3: raises
    os.listdir("nonexistant")
  • D: raises
    rdmd --eval 'dirEntries("nonexistant", SpanMode.shallow).writeln'
  • C++: raises
#include <string>
#include <iostream>
#include <filesystem>
namespace fs = std::filesystem;
int main(){
    std::string path = "nonexistant";
    for (const auto & entry : fs::directory_iterator(path))
        std::cout << entry.path() << std::endl;
}
  • nodejs: raises
const { readdirSync, statSync } = require('fs')
const { join } = require('path')
const dirs = p => readdirSync(p).filter(f => statSync(join(p, f)).isDirectory())
dirs('nonexistant')
  • matlab/octave: raises

dir /tmp/d18

  • ruby: raises
    ruby -e 'print Dir.entries("/tmp/private")'

  • swift: raises

import Foundation
let fileManager = FileManager.default
let files = try fileManager.contentsOfDirectory(atPath:"/tmp/d18")
print(files)
  • shell(ls or find): gives error

  • C also gives error code
    etc...

@timotheecour timotheecour changed the title walkDir, walkDirRec,listFiles now throw if input dir is invalid [RFC] walkDir, walkDirRec,listFiles now throw if input dir is invalid Mar 19, 2020
@Araq
Copy link
Member

Araq commented Mar 19, 2020

that's a racy condition, and also doesn't work:

Well that's your very own bugfix inside compiler/nimbledir. And also how you deal with the changed behaviour inside Nimble in preparation for this PR. It's a big breaking change and it breaks both the Nim compiler and Nimble.

The current behavior is downright error-prone and source of bugs (try: for ai in walkDir(getEnv("MISSPELLED_ENV_VAR")): echo ai),

Well APIs taking strings are inherently prone to typos and by the PR's logic getEnv should then also grow a check = false default parameter...

Look, if Nimble processed 0 files inside tests/ it should report that and in fact, it should report the number of tests it did run anyway. By your logic an empty tests/ directory does not raise an exception so Nimble continues with its weird behaviour but a non-existing directory causes Nimble to produce an error message. Not good enough considering the code breakage it causes.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 20, 2020

Well that's your very own bugfix inside compiler/nimbledir

I used existsDir in nim-lang/nimble#780 precisely because there is no simple and correct alternative prior to this PR #13642 that I'm aware of to check whether a directory is readable. For example the following code:

proc canWalkDir*(dir: string): bool =
  if not dirExists(dir): return false
  let perms = getFilePermissions(dir)
  return canPidAccessDir(getCurrentProcessId(), fpUserRead in perms, fpGroupRead in perms, fpOthersRead in perms)

this is fraught with caveats, eg:

  • race conditions
  • doesn't work on windows (On Windows, only the readonly flag is checked, every other permission is available in any case. so the directory may be denied for listing for a user but getFilePermissions would still return fpUserRead) and probably other OS specific ACL's
  • canPidAccessDir is tricky to implement, especially making it work cross platform (setuid etc have to be taken into account)

After this PR, all you'll need is: walkDir(dir, checkDir=true or false) depending on whether you want to ignore errors or not. I can't update the PR (dom closed it) but once he reopens I can update to something like:

# correct fix to nimble:
when defined(nimHasWalkDirRaises): walkDir(dir, checkDir = false)
else: walkDir(dir)

which preserves pre-existing semantics 100%.

It's a big breaking change and it breaks both the Nim compiler and Nimble

how so? honest question. I don't see how the correct fix to nimble i showed above is a breaking change; it should have identical semantics compared to prior my 2 PR's (this + nimble)

getEnv should then also grow a check = false default parameter.

I mentioned no such thing here, and hasEnv at least is available and reliable here

a non-existing directory causes Nimble to produce an error message. Not good enough considering the code breakage it causes.

no, that's not what my PR nim-lang/nimble#780 does; I'm using dirExists (or, after PR gets re-opened, I'll use correct fix to nimble snippet), which ignores any such error.
There is no code breakage here, the pre-existing semantics used by nimble are preserved.

And finally, you've already agreed that raising when top-level dir was invalid was the right default:
#13011 (comment)

Good points, I agree.

And you had agreed 8 days ago here too: #12676 by merging bbc231f (even though that PR didn't actually fix anything, unlike this PR).

There's a good reason why pretty much every language I've ever used raises by default when top-level dir is invalid:

  • python
  • D
  • C, C++
  • nodejs
  • ruby
  • swift
  • matlab/octave
  • shell(ls, find)
  • ...

silently ignoring errors causes bugs and shouldn't be the default.

@timotheecour timotheecour changed the title [RFC] walkDir, walkDirRec,listFiles now throw if input dir is invalid [RFC] walkDir+friends now throw if input dir is invalid, like almost all other languages Mar 20, 2020
@timotheecour timotheecour changed the title [RFC] walkDir+friends now throw if input dir is invalid, like almost all other languages [RFC] walkDir+friends now raises if input dir is invalid, like almost all other languages Mar 20, 2020
@Araq
Copy link
Member

Araq commented Mar 20, 2020

  1. I agree this new behaviour is better but it's a breaking change so it's needs a deprecation period / migration path. It's a breaking API change! I am well aware that after your PR Nimble behaves like before.
  2. Now you're supposing a different API where walkDir has a new flag. I like this better but the migration path is unclear, better would be to deprecate walkDir and introduce a better walkDir under a different name, say dirs or whatever.

@narimiran
Copy link
Member

@timotheecour I talked to @Araq and based on our discussion, I've pushed some changes.

P.S. For future: please don't disable packages like you did here. Comment them out.

@narimiran narimiran changed the title [RFC] walkDir+friends now raises if input dir is invalid, like almost all other languages [RFC] 'walkDir' now has a new 'checkDir' flag, to mimic behaviour of other languages Mar 20, 2020
changelog.md Outdated Show resolved Hide resolved
@narimiran narimiran merged commit 1d665ad into nim-lang:devel Mar 20, 2020
@timotheecour
Copy link
Member Author

P.S. For future: please don't disable packages like you did here. Comment them out.

  • disabling a package (by commenting it out) means nothing gets tested at all for this package which is worse for regression testing.
  • adding cmd = "" as I had done means if package later adds tests it won't be tested by testament (without further modification to important_packages), so it's not ideal indeed, I concede.

The problem remains that some packages appear tested but really aren't being tested at all, as I've explained here #13642 (comment):

eg see CI logs for this PR https://github.com/nim-lang/Nim/runs/522044580

=> 0 indication that something fishy was going on.

proposal

@narimiran
Copy link
Member

I'll keep this short:

Adding cmd = "" means that we won't notice that package testing is disabled/limited when we quickly scan through the important_packages.nim. We'll notice if something is commented out, as it clearly stands out.

Thank you for your cooperation.

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.

Nimscript listFiles should throw exception when path is not found
6 participants