bug: Inconsistent current directory resolution depending on include order #1757
Conversation
taskfile/reader.go
Outdated
| // the tests pass, which suggests a data race as the culprit. | ||
| // | ||
| // Uncomment this to make path-resolution bug go away | ||
| // g.SetLimit(1) |
There was a problem hiding this comment.
Uncommenting this will stop the bug from happening.
pbitty
changed the title
|
I've been able to narrow down the cause to the way the includes are parsed. I haven't figured out the root cause, but it seems when that having multiple includes with the same By disabling concurrency and putting the include with an empty I will keep investigating, but if anyone spots the issue in the meantime let me know. |
|
@andreynering @vmaerten , I believe I have figured out the issue. The relative path resolution in HTTP nodes only works in some scenarios because it always looks "one folder up". I believe the desired result is to make all paths relative to the parent. If the parent is a File node, it will stop at the file node. If the parent is a HTTP node, that node will have taken the value from its parent, and so on. Does this sound right to you? I am still familiarizing myself with the graph merging logic, so it's possible I am misunderstanding what is happening. One thing I still don't fully understand is why the ordering matters. It seems that the first include in |
|
I'm still not entirely sure what the expected behaviour is. I've posted a question about it in Discourse - will gladly move the discussion here if that's more appropriate. |
|
Whoops, I did it again - I forgot to enable remote taskfile support in the tests. If no objections, I will cherry-pick the change I did in ec64761 and then resolve any merge conflicts if both PRs are to be merged. |
testdata/includes_http/root-taskfile-remotefile-empty-dir-1st.yml
Outdated
Show resolved
Hide resolved
|
@pd93 I'll leave this PR open for a few days before merging it, in case you want to review it. |
|
@vmaerten Thanks! It's at the top of my list |
|
It looks good to me, so I'm merging. Thanks @pbitty! |
It seems there's a race condition somewhere in the
Reader.include()code.I was exploring the behaviour of the
includes.*.diroptions and I wrote some tests. When I did this, I found there two different behaviours that it would switch between intermittently. I tried to capture what I think is the expected behaviour in the tests and when you run the tests they will pass about 50% of the time.I found that if I force the "include loop" to not have any concurrency, the tests pass 100% of the time.
I have tried to play around with the variable assignments and mutations that happen inside
include, to see if I can isolate the cause, but no luck yet. In this PR I am posting only the tests and the concurrency fix (commented out), to highlight the problem and see if anyone has any ideas as to what's causing it.I will continue to explore it - but if anyone has any ideas, let me know.