Skip to content

Commit

Permalink
Sort directories visited by for_recursive_glob to make the results co…
Browse files Browse the repository at this point in the history 
…nsistent on different file systems.
  • Loading branch information
@resuna
resuna committed Mar 29, 2020
1 parent f9bed86 commit ff113e8
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion library/globrecur.tcl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ proc for_recursive_glob {var dirlist globlist cmd {depth 1}} {
return -code $code $result
}

foreach file [readdir $dir] {
foreach file [lsort [readdir $dir]] {
set file [file join $dir $file]
if [file isdirectory $file] {
set fileTail [file tail $file]
Expand Down

12 comments on commit ff113e8

@ramikhaldi
Copy link

Choose a reason for hiding this comment

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

I think, if you remove the lsort here, the build will pass (I could see that with PR12).

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This was added specifically to make the tests pass. What specifically is it making fail?

@ramikhaldi
Copy link

Choose a reason for hiding this comment

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

Could you reproduce this issue on any other machine ?

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What specifically is the failure mode that adding the lsort creates for you, on what platform?

You can simply copy and paste the errors.

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020

Choose a reason for hiding this comment

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

We only saw the problem on linux containers using tmpfs where readdir did not automatically return sorted results.

@ramikhaldi
Copy link

Choose a reason for hiding this comment

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

What specifically is the failure mode that adding the lsort creates for you, on what platform?

I couldn't observe any failures on my machine (adding/removing lsort had no effects on test results on my machine :) )

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020

Choose a reason for hiding this comment

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

Then why do you want to remove it?

@ramikhaldi
Copy link

@ramikhaldi ramikhaldi commented on ff113e8 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Then why do you want to remove it?

Because it caused errors on the ci.

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020

Choose a reason for hiding this comment

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

What errors does it cause you on the ci? That is the failure mdoe I am asking about.

@ramikhaldi
Copy link

@ramikhaldi ramikhaldi commented on ff113e8 May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What errors does it cause you on the ci? That is the failure mdoe I am asking about.

https://travis-ci.org/github/flightaware/tclx/jobs/686627674

That was the checkin when I added the lsort command. After removing the lsort, the build has succeded:

https://travis-ci.org/github/flightaware/tclx/builds/686631489

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020

Choose a reason for hiding this comment

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

We have seen the ---- errorInfo: User CPU time does not appear to be accumulating failure before. It appears to be due to github's tests being run on a potato. Repeat attempts eventually succeeded.

@resuna
Copy link
Member Author

@resuna resuna commented on ff113e8 May 13, 2020

Choose a reason for hiding this comment

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

profile.test doesn't even call glob.

Please sign in to comment.