-
-
Notifications
You must be signed in to change notification settings - Fork 624
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
#209 When "dir:" attribute points to a non-existing dir, create it #211
Conversation
Ah, I didn't realize that CI failed. |
@andreynering I fixed the embarrassing CI failure ;-) |
task.go
Outdated
// If so, we create it. | ||
if t.Dir != "" { | ||
if _, err := os.Stat(t.Dir); os.IsNotExist(err) { | ||
if err := os.MkdirAll(t.Dir, 0755); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps move this code to Executor.Setup()
?
That would prevent race conditions if the same task is ran in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mhh, I am not very familiar with the internals of task, but here is what I found:
- To create the directory, we need attribute
t.Dir
. At the very beginning ofRunTask()
, we callCompiledTask()
Line 178 in 9c475c3
t, err := e.CompiledTask(call) t.Dir
If we were to move this code to Setup()
, we would have to do the following:
- Add a loop that goes through all the tasks, so that it can look at each
call
of typetaskfile.Call
, callt := CompiledTask(call)
and then create the directory based ont.Dir
? This would mean also that it would create directories also for tasks that are not required on the command line (directly or indirectly via dependencies).
If my understanding is right, then I need some more guidance on how to do this properly :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking: if there is a race condition in calling RunTask()
, then this race condition is also there independently from this PR, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @marco-m,
You're right that it'd be more complicated. I totally forgot that we'd need to compile each task.
I think we can then just have a mutex/lock for that. Extracting into another function would be nice, too.
func (t *Task) mkdir() error {
if t.Dir == "" {
return nil
}
t.mkdirMutex.Lock()
defer t.mkdirMutex.Unlock()
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreynering I can do that. But what I am missing is this: the mutex would protect for the specific race when creating a directory, but it looks like there is an inherent race when task run in parallel? Wouldn't make more sense to put the mutex outside this new mkdir() function ?
@andreynering I added the mutex, but please see my questions above about the inherent race. |
ping |
Hi @marco-m, This is merged. I did some small changes on fe2b8c8. About your question above: tasks are supposed to run in parallel, and there's no race condition in Task itself, unless the user programs a task that won't work if ran in parallel. But since the dir creation is Task work, let's keep the mutex to keep things safe. 🙂 |
Thank you! |
I see you moved the mutex code. Yes, I was unsure where to put it. |
Document dir: creation (see PR #211)
Also:
Closes #209