[superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably #10545

timotheecour · 2019-02-04T05:05:59Z

parentDir, parentDirs now work reliably

fix [TODO] bugs with parentDir #8734
fix [ospaths] toSeq(parentDirs("/a/b")) returns @["/a/b", "/a", ""] instead of @["/a/b", "/a", "/"] (last entry wrong) #8353
removes workaround in ./compiler/packagehandling.nim

XXX os's parentDirs is stupid (multiple yields) and triggers an old bug...

many other issues I saw while working on the fix.

I also added extensive unittests so you can now see exactly what parentDir and parentDirs return

to view the diff and not get confused by code that was only moved, see instructions here https://github.com/nim-lang/Nim/pull/10435/files

note

the # todo: fix is a (commented out) test case for a pre-existing failure from #8395 which i can fix after this PR

… reliably

Araq

#8734 is arguably not a bug. Especially not if you keep the behaviour that an empty string signals a wrong usage.

lib/pure/os.nim

compiler/packagehandling.nim

tests/stdlib/tos.nim

timotheecour · 2019-02-04T12:13:06Z

#8734 is arguably not a bug. Especially not if you keep the behaviour that an empty string signals a wrong usage.

#8734 is not 1 bug but multiple bugs and as discussed here https://irclogs.nim-lang.org/04-02-2019.html#10:56:02 and elsewhere in this conversation thread some of the items are indeed bugs, and 1 of them is being debated.
So i added a simple CT switch (that is currently set to parentDirOfRootIsEmpty = true ) to unblock this PR and we can address that point later, which'll only require resetting parentDirOfRootIsEmpty = false if we decide to go with that

timotheecour · 2019-02-04T21:24:33Z

@Araq PTAL all comments addressed

Araq · 2019-02-05T14:14:33Z

tests/stdlib/tos.nim

+    ("/a.txt", "/"),
+  ]
+
+  doAssert runTestCases("parentDir", examples, parentDir(it))


That is cargo-cult programming. Now I can look at a list of tuples all day long before I can see it's about parentDir. DRY and meta programming are for creating higher abstractions, not to save keystrokes.

Now I can look at a list of tuples all day long before I can see it's about parentDir

I just pushed a commit that adds documentation to make everything clear.

I had initially started with the one doAssert per test case but that became quickly un-manageable with the number of test cases, in particular:

the important part of each test case was lost in noise of the repetitive surrounding code, eg doAssert parentDir("/usr/local/bin".unixToNativePath) == "/usr/local".unixToNativePath ; instead, I'm only highlighting the part that changes

on failure, it doesn't tell you what parentDir produced as output (because you don't like assertEquals for some reason)

it stops on 1st error instead of showing all errors, making it terrible when you need to change the implementation of parentDirs; instead I'm showing all errors before failing

if you have to change something, you have to change it everywhere; unlike here where I need to change it in only one place. yes, it's DRY, it's not cargo cult

note:

the lack of this kind of thorough testing as done here for parentDir is the root cause for all those issues and regressions I keep running into when using std/os procs (eg #10018 , #10047 , etc)

I have other PR's in the pipeline that fix other bugs in std/os (eg "/a" /../ "d" should be "/d", not "d" + other bugs with other procs) so let's please move on with this PR

the lack of this kind of thorough testing as done here for parentDir is the root cause for all those issues and regressions I keep running into when using std/os procs (eg #10018 , #10047 , etc)

Maybe but it's just you. Most people don't use double slashes in all their paths. And your improved implementation adds plenty of its own, new problems.

@Araq

I don't understand your comment. [os] fix #10017 regression, fix #10025 regression #10018, fix #8255 numerous issues with splitFile #10047, [superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably #10545 and other PRs I wrote for path handling fix bugs and regressions unrelated to double slashes; incorrect handling of paths with double slashes were just one of the issues I've fixed.

all languages I've worked with correctly handle paths with double slashes, eg: C++(boost), D, python, java, js(node) etc...; as I already explained

Double slashes can occur from a variety of reasons, eg:

export buildDir=/path/to/build/ export logFile=$buildDir/log.txt # => expands to a path with double slash (`${buildDir}log.txt` in contrast only works if `buildDir` ends with a slash)

see also https://unix.stackexchange.com/questions/212805/should-i-use-a-slash-at-the-end-of-path-variables-in-shell-script-or-not

To be on the safe side, include the slash. This can lead to multiple slashes when concatenating the paths, but at least you avoid problems.

and this: https://unix.stackexchange.com/questions/1910/how-does-linux-handle-multiple-consecutive-path-separators-home-username

Multiple slashes are allowed and are equivalent to a single slash

And your improved implementation adds plenty of its own, new problems

I'm not sure which are these new problems you're referring to, after this PR was updated to return "" instead of / for parentDir("/"), more precisely: what previously valid code would become incorrect as a result of this PR.

In any case, as mentioned below, I'm following up with #10633

tests/stdlib/tos.nim

Araq · 2019-02-05T14:17:14Z

tests/stdlib/tos.nim

@@ -26,6 +26,31 @@ Raises
 # test os path creation, iteration, and deletion

 import os, strutils, pathnorm
+import "$nim/compiler/unittest_light"


unittest is bad because it's full of when compiles. Guess what, unittest_light repeats the mistake. The tests were fine before without this import.

I removed that import for the sake of moving fwd with this PR, but please explain what is wrong with unittest_light having compiles ?

assertEquals is written as if lhs!=rhs: doAssert false, mismatch(lhs, rhs) so the compiles is just used for printing the error msg (inside mismatch), not for correctness validation

theres is already a test for that tests/compiler/tunittest_light.nim which tests assertEquals using doAssert so if assertEquals had bugs, doAssert would catch it

If it's a compilation speed issue, there are easy options.

More generally, for assertEquals, unittest.check, fmt, and sequtils: 1 argument I heard against more widespread use in testament was "who is guarding the guardian" argument; I'm all for adding more thorough testing (as needed) for these but we shouldn't use FUD arguments that they may have bugs that would render testament useless;
If a bug ever slips through existing checks, it'll eventually be found in the wild, reported, and fixed;
note, outside of testament, these are pretty popular tools for testing in nimble packages, for a reason.

nim-regex doesn't compile with devel because it uses unittest and unittest does nasty stuff. Yeah, these are compiler regressions, but still, a testing infrastructure shouldn't stress the compiler so much that it becomes hard to debug itself. No FUD here, just lots of experience with unittest.

the concerns you're raising seem specific to unittest, not unittest_light, as I described here #10545 (comment)

lib/pure/os.nim

timotheecour · 2019-02-06T02:53:17Z

@Araq comments addressed and green, PTAL

Araq · 2019-02-06T09:00:33Z

lib/pure/os.nim

+      assert parentDir("/") == ""
+  if path == "": return ""
+  result = path
+  while true:


Convoluted. What's wrong with fixing parentDirPos?

Araq · 2019-02-06T09:01:10Z

lib/pure/os.nim

+  result = path
+  while true:
+    normalizePathEnd(result)
+    let (dir, name, ext) = splitFile(result)


Why would I do that? What has ext to do with anything?

Araq · 2019-02-06T09:02:08Z

lib/pure/os.nim

+  while true:
+    normalizePathEnd(result)
+    let (dir, name, ext) = splitFile(result)
+    if name == ".." or (name.len == 0 and ext.len == 0): return ""


parentDir("bar/baz/foo/..") could just work...

Araq · 2019-02-06T09:02:44Z

lib/pure/os.nim

+    normalizePathEnd(result)
+    let (dir, name, ext) = splitFile(result)
+    if name == ".." or (name.len == 0 and ext.len == 0): return ""
+    if name == ".":


Ah so silly single dots are allowed.

Araq · 2019-02-06T09:03:28Z

lib/pure/os.nim

+      result = dir
+      continue
+    if dir == "" and name != "":
+      return "."


Really? What is this "." directory and if it is one why is ".." not a directory?

Araq · 2019-02-06T09:05:11Z

lib/pure/os.nim

+      continue
+    if dir == "" and name != "":
+      return "."
+    return dir


At this point in the control flow it's not obvious if it shouldn't have been return result.

krux02 · 2019-02-07T08:51:04Z

too long didn't read.

timotheecour · 2019-02-11T08:23:57Z

instead of addressing remaining comments in this PR I'm opening #10633 which has a different, simpler implementation (some edge cases are treated differently) and should address the remaining comments from here; if #10633 gets merged I'll close this PR but preferably not before because github doesn't allow re-opening a closed PR after a force push.

krux02 · 2019-02-11T13:34:17Z

this I am closing this in favor of #10633

timotheecour force-pushed the pr_fix_parentDir branch from 708be8a to 9ffaf16 Compare February 4, 2019 05:12

timotheecour added 2 commits February 3, 2019 22:46

[os] fix nim-lang#8734, nim-lang#8353: parentDir, parentDirs now work…

d1bd997

… reliably

convert parentDirs to runnableExamples

1b7d853

timotheecour force-pushed the pr_fix_parentDir branch from 4e6d505 to d2ffb23 Compare February 4, 2019 06:46

refactor using runTestCases

d3d8d82

timotheecour mentioned this pull request Feb 4, 2019

os.tailDir fails on some paths #8395

Closed

timotheecour force-pushed the pr_fix_parentDir branch from d2ffb23 to bb4401d Compare February 4, 2019 08:20

timotheecour added 2 commits February 4, 2019 01:21

fix bug

90ec726

packagehandling: addresses "parentDirs is stupid"

d891765

timotheecour force-pushed the pr_fix_parentDir branch from bb4401d to d891765 Compare February 4, 2019 09:21

timotheecour requested a review from Araq February 4, 2019 10:05

Araq reviewed Feb 4, 2019

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

compiler/packagehandling.nim Outdated Show resolved Hide resolved

tests/stdlib/tos.nim Show resolved Hide resolved

add parentDirOfRootIsEmpty pending debate is resolved

2a256ac

address comments

64818ba

This was referenced Feb 4, 2019

[ci skip] document that multiple yield in inline iterator cause code bloat #10553

Merged

os: bugs with /../ #10555

Open

timotheecour added the OS (stdlib) Related to OS modules in standard library label Feb 5, 2019

timotheecour mentioned this pull request Feb 5, 2019

[TODO] os.parentDirs now uses a single yield to avoid code bloat; refs #10553 #10557

Closed

Araq reviewed Feb 5, 2019

View reviewed changes

tests/stdlib/tos.nim Outdated Show resolved Hide resolved

Araq reviewed Feb 5, 2019

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

Araq reviewed Feb 5, 2019

View reviewed changes

lib/pure/os.nim Outdated Show resolved Hide resolved

timotheecour added 5 commits February 5, 2019 12:58

fixup

7a653a0

bake in parentDirOfRootIsEmpty=true assumption

e512770

fix typo in splitFile

bab5e9d

removed unittest_light, worked around it

aaa04b8

improve documentation for runTestCases use

34c8005

Araq reviewed Feb 6, 2019

View reviewed changes

timotheecour mentioned this pull request Feb 11, 2019

[TODO] parentDir, parentDirs now work reliably; fix #8734, #8353 (simpler fix) #10633

Closed

timotheecour changed the title ~~[os] fix #8734, #8353: parentDir, parentDirs now work reliably~~ [superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably Feb 11, 2019

krux02 closed this Feb 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably #10545

[superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably #10545

timotheecour commented Feb 4, 2019 •

edited

Loading

Araq left a comment

timotheecour commented Feb 4, 2019

timotheecour commented Feb 4, 2019

Araq Feb 5, 2019

timotheecour Feb 5, 2019 •

edited

Loading

Araq Feb 6, 2019

timotheecour Feb 11, 2019 •

edited

Loading

Araq Feb 5, 2019

timotheecour Feb 5, 2019

Araq Feb 6, 2019

timotheecour Feb 11, 2019

timotheecour commented Feb 6, 2019

Araq Feb 6, 2019

Araq Feb 6, 2019

Araq Feb 6, 2019

Araq Feb 6, 2019

Araq Feb 6, 2019

Araq Feb 6, 2019

krux02 commented Feb 7, 2019

timotheecour commented Feb 11, 2019 •

edited

Loading

krux02 commented Feb 11, 2019

[superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably #10545

[superseded] [os] fix #8734, #8353: parentDir, parentDirs now work reliably #10545

Conversation

timotheecour commented Feb 4, 2019 • edited Loading

note

Araq left a comment

Choose a reason for hiding this comment

timotheecour commented Feb 4, 2019

timotheecour commented Feb 4, 2019

Choose a reason for hiding this comment

timotheecour Feb 5, 2019 • edited Loading

Choose a reason for hiding this comment

note:

Choose a reason for hiding this comment

timotheecour Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheecour commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krux02 commented Feb 7, 2019

timotheecour commented Feb 11, 2019 • edited Loading

krux02 commented Feb 11, 2019

timotheecour commented Feb 4, 2019 •

edited

Loading

timotheecour Feb 5, 2019 •

edited

Loading

timotheecour Feb 11, 2019 •

edited

Loading

timotheecour commented Feb 11, 2019 •

edited

Loading