public inbox for goredo-devel@lists.cypherpunks.ru
Atom feed
* [PATCH] Don't create nested target directories automatically
@ 2022-04-15 0:55 Andrew Chambers
2022-04-15 6:10 ` goredo
2022-04-15 16:23 ` Sergey Matveev
0 siblings, 2 replies; 15+ messages in thread
From: Andrew Chambers @ 2022-04-15 0:55 UTC (permalink / raw)
To: goredo-devel; +Cc: Andrew Chambers
If a user mistypes a rule, goredo creates those directories automatically
leaving junk on the filesystem. This change means higher level rules must
create those directories explicitly if needed.
---
run.go | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/run.go b/run.go
index 32e3800..1a657a3 100644
--- a/run.go
+++ b/run.go
@@ -123,13 +123,6 @@ func (e RunError) Error() string {
return fmt.Sprintf("%s: %s", e.Name(), e.Err)
}
-func mkdirs(pth string) error {
- if _, err := os.Stat(pth); err == nil {
- return nil
- }
- return os.MkdirAll(pth, os.FileMode(0777))
-}
-
func isModified(cwd, redoDir, tgt string) (bool, *Inode, string, error) {
fdDep, err := os.Open(path.Join(redoDir, tgt+DepSuffix))
if err != nil {
@@ -190,7 +183,11 @@ func syncDir(dir string) error {
func runScript(tgtOrig string, errs chan error, traced bool) error {
cwd, tgt := cwdAndTgt(tgtOrig)
redoDir := path.Join(cwd, RedoDir)
- if err := mkdirs(redoDir); err != nil {
+
+ if err := os.Mkdir(redoDir, os.FileMode(0777)); err != nil && !os.IsExist(err) {
+ if os.IsNotExist(err) {
+ return TgtError{tgtOrig, fmt.Errorf("target directory does not exist")}
+ }
return TgtError{tgtOrig, err}
}
--
2.33.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] Don't create nested target directories automatically
2022-04-15 0:55 [PATCH] Don't create nested target directories automatically Andrew Chambers
@ 2022-04-15 6:10 ` goredo
2022-04-15 6:49 ` Andrew Chambers
2022-04-15 6:57 ` Andrew Chambers
2022-04-15 16:23 ` Sergey Matveev
1 sibling, 2 replies; 15+ messages in thread
From: goredo @ 2022-04-15 6:10 UTC (permalink / raw)
To: goredo-devel
It was a concious decision that redo creates target directories. Now, every default*.do file must be aware of file hierarchies, which is quite annoying.
It also doesn't solve your problem, because, now, your default.do file also contains a line like:
`mkdir -p $(dirname $2)`.
This means, the garbage directory is also created for your mistyped target unless you take special precautions in all of your default.do files to limit their scope of directories they are concerned with. (before you call mkdir) You cannot and you don't always want to do this. E.g. in a very general default.o.do to compile a C file.
The general contract with redo is, that the position of the .do file in the directory hierarchy limits its scope.
Regards,
–Michael
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 6:10 ` goredo
@ 2022-04-15 6:49 ` Andrew Chambers
2022-04-15 7:15 ` goredo
2022-04-15 16:23 ` Sergey Matveev
2022-04-15 6:57 ` Andrew Chambers
1 sibling, 2 replies; 15+ messages in thread
From: Andrew Chambers @ 2022-04-15 6:49 UTC (permalink / raw)
To: goredo-devel
> It was a concious decision that redo creates target directories. Now, every default*.do file must be aware of file hierarchies, which is quite annoying.
>
What you are describing is incompatible with apenwarr redo and redo-c.
Those do not create the directories so any do file that takes advantage
of this is not portable.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 6:10 ` goredo
2022-04-15 6:49 ` Andrew Chambers
@ 2022-04-15 6:57 ` Andrew Chambers
1 sibling, 0 replies; 15+ messages in thread
From: Andrew Chambers @ 2022-04-15 6:57 UTC (permalink / raw)
To: goredo-devel
> It also doesn't solve your problem, because, now, your default.do file also contains a line like:
> `mkdir -p $(dirname $2)`.
This is not the case in my do files, they do not arbitrarily call mkdir
without first checking if the target is something they know how to build.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 6:49 ` Andrew Chambers
@ 2022-04-15 7:15 ` goredo
2022-04-15 7:31 ` Andrew Chambers
2022-04-15 16:23 ` Sergey Matveev
1 sibling, 1 reply; 15+ messages in thread
From: goredo @ 2022-04-15 7:15 UTC (permalink / raw)
To: goredo-devel
You're right. They don't. Still one problem remains.
Goredo also puts $3 in the target directory. So, this also means it is now a hard error to write to stdout (it points to $3) without first creating the directory. How does apenwarr handle this?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 7:15 ` goredo
@ 2022-04-15 7:31 ` Andrew Chambers
2022-04-15 9:28 ` goredo
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Chambers @ 2022-04-15 7:31 UTC (permalink / raw)
To: goredo-devel
> You're right. They don't. Still one problem remains.
>
> Goredo also puts $3 in the target directory. So, this also means it is now a hard error to write to stdout (it points to $3) without first creating the directory. How does apenwarr handle this?
>
I do not think you can even attempt to build a target that does not
already have a directory, it just reports an error.
I suppose this is a problem if for example you do not wish your .o files
to exist beside your .c files. You might need to put mkdir -p calls into
a top level 'all' rule (which I admit you might not have). That being
said, I don't think redo ever supported this sort of 'out of tree' build
very well to begin with.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 7:31 ` Andrew Chambers
@ 2022-04-15 9:28 ` goredo
2022-04-15 12:00 ` Andrew Chambers
0 siblings, 1 reply; 15+ messages in thread
From: goredo @ 2022-04-15 9:28 UTC (permalink / raw)
To: goredo-devel
I tried apenwarr and it works on targets in non-existent directories. I remember another, I think it was redo-sh, which failed.
Redo implementations that don't work on targets in non-existent directories are dysfunctional, in my opinion. The reason is that a) you cannot depend on directories, only files and b) redo's discovery algorithm does not allow you to build targets from higher levels to lower levels. When you call redo a/b/c, the target a/b/c is called first and may or may not depend on another target that produces directories a/b/ and a/. If target a/b/c fails immediately, the others are never called.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 9:28 ` goredo
@ 2022-04-15 12:00 ` Andrew Chambers
2022-04-15 13:17 ` goredo
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Chambers @ 2022-04-15 12:00 UTC (permalink / raw)
To: goredo-devel
On 4/15/22 21:28, goredo wrote:
> I tried apenwarr and it works on targets in non-existent directories. I remember another, I think it was redo-sh, which failed.
I have a test here that disagrees with you:
echo "echo foo" > default.do
redo ./blah/blah/blah
redo blah/blah
redo: ./blah/blah: target dir '/tmp/test/blah' does not exist!
redo blah/blah (exit 209)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 12:00 ` Andrew Chambers
@ 2022-04-15 13:17 ` goredo
2022-04-15 13:39 ` Andrew Chambers
2022-04-15 16:23 ` Sergey Matveev
0 siblings, 2 replies; 15+ messages in thread
From: goredo @ 2022-04-15 13:17 UTC (permalink / raw)
To: goredo-devel
I may have misunderstood you. I thought, you wanted redo to fail immediately, once it is called on a target in an non-existent directory. Okay, the contract, then, would be, that you'd have to create the directory before writing. Which, of course, is reasonable.
Next situation: In goredo, it is legal and wanted, that you can implement always-out-of-date targets by not creating any output. You would still need the directory to store the record for that target. And it would not be obvious, why you'd have to have a mkdir call, when you don't want to write something. Those would also pollute apenwarr user's trees, who don't have to do that.
One suggestion to your patch: Instead of failing, you could defer creating the directory to the point where redo actually wants to write a record to a file.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 13:17 ` goredo
@ 2022-04-15 13:39 ` Andrew Chambers
2022-04-15 13:58 ` goredo
2022-04-15 16:23 ` Sergey Matveev
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Chambers @ 2022-04-15 13:39 UTC (permalink / raw)
To: goredo-devel
> I may have misunderstood you. I thought, you wanted redo to fail immediately, once it is called on a target in an non-existent directory. Okay, the contract, then, would be, that you'd have to create the directory before writing. Which, of course, is reasonable.
>
>
You didn't misunderstand me... My example shows apenwarr redo fails when
run on a target in a non existent directory. By the way @spacefrogg -
are you the main developer of goredo? your email address does not match
the git commits in the repository.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 13:39 ` Andrew Chambers
@ 2022-04-15 13:58 ` goredo
0 siblings, 0 replies; 15+ messages in thread
From: goredo @ 2022-04-15 13:58 UTC (permalink / raw)
To: goredo-devel
I am not the developer.
The following works with apenwarr on any file in any hierarchy:
default.do:
mkdir -p $(dirname $2)
touch $3
Same goes for goredo and your patch but not redo-sh (AFAIR).
It differs from goredo in the following aspect for a phony target:
default.do:
redo-ifchange foo bar baz $(dirname $2)/file.ext
This do file works in apenwarr for a/b/all and creates no directories. It works in current goredo and does create directories. Will it work with your patch? It needs the directories to make a record in a/b/.redo.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 0:55 [PATCH] Don't create nested target directories automatically Andrew Chambers
2022-04-15 6:10 ` goredo
@ 2022-04-15 16:23 ` Sergey Matveev
1 sibling, 0 replies; 15+ messages in thread
From: Sergey Matveev @ 2022-04-15 16:23 UTC (permalink / raw)
To: goredo-devel
[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]
*** Andrew Chambers [2022-04-15 12:55]:
>If a user mistypes a rule, goredo creates those directories automatically
>leaving junk on the filesystem. This change means higher level rules must
>create those directories explicitly if needed.
Actually I was aware of automatic mistyped directories creation when I
talked with the baredo's author. And actually my first intent was
prohibiting "mkdir -p" behaviour indeed too. And I also gave the same
suggestion to use "mkdir -p $2:h"-analogue in default.do-files. But
baredo's author calls that "mkdir -p"-like behaviour a feature, very
convenient feature other implementation lacks and he explicitly added
that behaviour to his implementation too.
Personally I have never mistyped my targets (do not know why), so have
never noticed that unexpectedly left garbage. But if users find that
side-effect as a feature, not a bug, I decided to leave it as it is.
--
Sergey Matveev (http://www.stargrave.org/)
OpenPGP: CF60 E89A 5923 1E76 E263 6422 AE1A 8109 E498 57EF
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 6:49 ` Andrew Chambers
2022-04-15 7:15 ` goredo
@ 2022-04-15 16:23 ` Sergey Matveev
1 sibling, 0 replies; 15+ messages in thread
From: Sergey Matveev @ 2022-04-15 16:23 UTC (permalink / raw)
To: goredo-devel
[-- Attachment #1: Type: text/plain, Size: 911 bytes --]
*** Andrew Chambers [2022-04-15 18:49]:
>What you are describing is incompatible with apenwarr redo and redo-c. Those
>do not create the directories so any do file that takes advantage of this is
>not portable.
Neither goredo, nor redo-c, not apenwarr/redo (as far as I remember)
explicitly tell how they behave with missing directories. So that can be
treated as an undefined behaviour. So portable .do-files just must not
rely on undefined behaviour. If someone explicitly expects for
directories to be created in his project, then he should explicitly
write that "mkdir -p" rule in his .do file and do not rely on some
undocumented sife-effects of some implementations (goredo). Portable and
correctly written .do files will behave the same way on all those three
implementations.
--
Sergey Matveev (http://www.stargrave.org/)
OpenPGP: CF60 E89A 5923 1E76 E263 6422 AE1A 8109 E498 57EF
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 13:17 ` goredo
2022-04-15 13:39 ` Andrew Chambers
@ 2022-04-15 16:23 ` Sergey Matveev
2022-04-16 0:36 ` Andrew Chambers
1 sibling, 1 reply; 15+ messages in thread
From: Sergey Matveev @ 2022-04-15 16:23 UTC (permalink / raw)
To: goredo-devel
[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]
*** goredo [2022-04-15 13:17]:
>Next situation: In goredo, it is legal and wanted, that you can implement always-out-of-date targets by not creating any output. You would still need the directory to store the record for that target. And it would not be obvious, why you'd have to have a mkdir call, when you don't want to write something.
Absolutely right! Phony targets will anyway just have to store at least
their "Build:"-identifier that is used to determine if the target was
already built during parallel jobs run. And it keeps ifcreate-dependencies
on .do-files. Phony targets are the first citizen dependencies as any
others too, they just do not has any content and are always out-of-date
because of that.
>One suggestion to your patch: Instead of failing, you could defer creating the directory to the point where redo actually wants to write a record to a file.
The only thing I see is really just to defer creating of dependency
information file until .do-file was really found. But if .do file is
run, then directories will be created anyway to store $3 and stdout
output. They can neither be deferred nor placed in other directories,
because they are renamed, that happens only within single filesystem.
So if .do fails, then anyway you will have directories and probably
empty .redo left.
Is it worth adding more complex code, just to remove some garbage if no
.do file was found? Anyway if you have got some default.do in
directories above, then the whole bunch of directories and .redo will be
created anyway, probably failing the target, leaving no $3/stdout files,
but directories are have to be created there. And you can not remove
them, checking if they are empty, because that will race with probably
running simultaneous jobs.
--
Sergey Matveev (http://www.stargrave.org/)
OpenPGP: CF60 E89A 5923 1E76 E263 6422 AE1A 8109 E498 57EF
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Don't create nested target directories automatically
2022-04-15 16:23 ` Sergey Matveev
@ 2022-04-16 0:36 ` Andrew Chambers
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Chambers @ 2022-04-16 0:36 UTC (permalink / raw)
To: goredo-devel
Thanks for all the clarification - I see its not so simple to address. I
will take some other precautions in my build tree to prevent people
doing this by accident.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-04-16 0:36 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 0:55 [PATCH] Don't create nested target directories automatically Andrew Chambers
2022-04-15 6:10 ` goredo
2022-04-15 6:49 ` Andrew Chambers
2022-04-15 7:15 ` goredo
2022-04-15 7:31 ` Andrew Chambers
2022-04-15 9:28 ` goredo
2022-04-15 12:00 ` Andrew Chambers
2022-04-15 13:17 ` goredo
2022-04-15 13:39 ` Andrew Chambers
2022-04-15 13:58 ` goredo
2022-04-15 16:23 ` Sergey Matveev
2022-04-16 0:36 ` Andrew Chambers
2022-04-15 16:23 ` Sergey Matveev
2022-04-15 6:57 ` Andrew Chambers
2022-04-15 16:23 ` Sergey Matveev