Fix creation of output directories ending with ..
#41
Conversation
pkg/builder/local_build_executor.go
Outdated
| @@ -241,7 +241,12 @@ func (be *localBuildExecutor) Execute(ctx context.Context, filePool re_filesyste | |||
| // there. We later use the directory handles to extract output files. | |||
| outputParentDirectories := map[string]filesystem.Directory{} | |||
| for _, outputDirectory := range command.OutputDirectories { | |||
| dirPath := path.Dir(outputDirectory) | |||
| var dirPath string | |||
There was a problem hiding this comment.
Could you move this part into a helper function?
| @@ -583,15 +583,23 @@ func TestLocalBuildExecutorWithWorkingDirectorySuccess(t *testing.T) { | |||
| // Creation of build/objects | |||
| rootDirectory := mock.NewMockDirectory(ctrl) | |||
| rootDirectory.EXPECT().Mkdir("build", os.FileMode(0777)).Return(nil) | |||
| rootDirectory.EXPECT().Mkdir("build", os.FileMode(0777)).Return(nil) | |||
There was a problem hiding this comment.
Shouldn't this in the second case return syscall.EEXIST?
| buildDirectory := mock.NewMockDirectory(ctrl) | ||
| rootDirectory.EXPECT().Enter("build").Return(buildDirectory, nil) | ||
| rootDirectory.EXPECT().Enter("build").Return(buildDirectory, nil) |
There was a problem hiding this comment.
Could you please keep this ordered a bit closer to what happens in practice? Also: maybe it's a bit more accurate to use two separate mocks for buildDirectory instead of one. In practice, it won't really happen that Enter() returns the same handle for both Enter() calls.
There was a problem hiding this comment.
As part of making this test a bit more realistic I'll probably move away from needing to call Enter("build") twice so that should handle this issue.
sdclarke
force-pushed
the
fix-relative-dirs
branch
from
5b38dbd
to
af32642
Compare
There was a problem hiding this comment.
Sorry for letting you wait. I had to give this some more thought.
We already have that workaround in place to create output directories up front, even though REv2 says we should not. Considering that output_directories is deprecated in favour of output_paths according to the latest version of the protocol, let's just assume that we'll never converge to the originally intended semantics anyway.
What are your thoughts on simply removing support for omitting output directories or letting them be symlinks? That would allow us to make a change like this to fix this specific bug:
https://github.com/buildbarn/bb-remote-execution/compare/issue-41
Note that I have only compile tested this change. The unit tests no longer pass. Could you give this a spin, and if it works integrate it into your branch? Thanks!
This is done by removing the code which creates the parent of the output directory and simply creating the directory itself directly as it was being created after the fact anyway
sdclarke
force-pushed
the
fix-relative-dirs
branch
from
af32642
to
570f589
Compare
I've integrated this and it still seems to work, I also fixed the failing unit tests. |
After #35 paths containing
..are now handled as defined by the REAPI spec, however a corner case has come up where a directory ending in..would end up being created and uploaded incorrectly due to the fact the determining the parent directory was done with a call topath.Dir. Extra logic has been added to deal with these cases in this pull request.Given that this is related to the changes made in #35 I have added testing for this behaviour to the test written in that pull request.