Skip to content
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

Add tests for moving nodes in fuse_fs #333

Closed
wants to merge 1 commit into from
Closed

Conversation

layus
Copy link
Contributor

@layus layus commented Oct 8, 2017

I was surprised to see that a simple mkdir a && mv a b does not work in fuse_fs.
The error is that node 'a' does not exist.
The second test does not pass ATM, it just outlines the bug

When moving files around, fuse_fs does not look in the list of temporary dirs.
This occurred while I was investigating the possibility to support generated directories.
It shows that directories are handled separately from generated files, increasing the required work to support generated directories.

Leaving generated directories aside, mkdir a && mv a b looks like something we would want to support anyway.

@gittup
Copy link
Owner

gittup commented Oct 11, 2017

I think you would just need a patch like this to support renaming a temporary directory:

diff --git a/src/tup/server/fuse_fs.c b/src/tup/server/fuse_fs.c
index 384915d..081c250 100644
--- a/src/tup/server/fuse_fs.c
+++ b/src/tup/server/fuse_fs.c
@@ -973,6 +973,20 @@ static int tup_fs_rename(const char *from, const char *to)
 
        finfo = get_finfo(to);
        if(finfo) {
+               struct tmpdir *tmpdir;
+               LIST_FOREACH(tmpdir, &finfo->tmpdir_list, list) {
+                       if(strcmp(tmpdir->dirname, peelfrom) == 0) {
+                               free(tmpdir->dirname);
+                               tmpdir->dirname = strdup(peelto);
+                               if(!tmpdir->dirname) {
+                                       perror("strdup");
+                                       put_finfo(finfo);
+                                       return -ENOMEM;
+                               }
+                               put_finfo(finfo);
+                               return 0;
+                       }
+               }
                /* If we are re-naming to a previously created file, then
                 * delete the old mapping. (eg: 'ar' will create an empty
                 * library, so we have one mapping, then create a new temp file

Though tup would still complain that the renamed directory was created if you don't rmdir it before the subprocess completes. Right now tup handles generated directories by creating them before the subprocess runs, so something like this:

: |> touch %o |> generated/out.txt

Would cause tup to mkdir("generated") before running "touch generated/out.txt". In other words, there shouldn't be a need for the subprocess itself to call the mkdir.

Does that not support your actual use-case? Can you elaborate on what you're trying to fix?

@layus
Copy link
Contributor Author

layus commented Oct 11, 2017

I feared that there may exist a simple fix like that, but that will indeed work for this use-case.

What I wanted is a more generic implementation of fuse_fs, where directories are considered as outputs, just like files are.
That way, a command could output a whole subtree, instead of just files whose name is known in advance. This would allow commands to output random file names, as long as they belong to a known output directory.

This does not change the way tup works, as commands reading or otherwise accessing that directory would get a dependency to the command that generates it.

In particular, I want to run cargo within tup (building gecko). But cargo generates hundreds of different files, and I see no way to ever list them explicitly as outputs, especially given that rustc writes libraries postfixed with their hash, so you basically need the output to know its name.

There seems to be ongoing effort (rust-lang/rfcs#2136) to make cargo compatible with strict build systems like bazel (~= tup in this context), but I think there will always be a case where you want to output a bunch of unknown files.

Bazel has a discussion about this, where they say that you should zip such outputs in a known archive. This is not sufficient. gecko for example has IPDL generation where one command generates a lot of output files. There may be a way to list the outputs, but it is much more convenient to just consider the directory tree as one big output.

Not sure i made my point here... Comments welcome :-)

@gittup
Copy link
Owner

gittup commented Oct 18, 2017

That is an interesting idea. I was considering something similar for the case of unknown outputs, where if you list a group as an output, then tup could allow access to the output file if other things depend on the group. The problem I ran into was how to handle wildcards properly, since they are currently handled in the parser when the outputs would still be unknown. It can probably be done with some caveats, but will require some work. I think it would basically be like your idea, but with groups instead of directories.

As for running cargo from gecko directly in tup, I don't think that will work well with tup's execution model. Assuming that we had unknown outputs supported, tup would see the cargo command as a giant monolith that has many inputs and many outputs. If any of the inputs change, tup would force rebuilding all of the outputs (they are removed before the subprocess executes). This means touching any Rust code would rebuild all of the Rust code in this case.

What we're trying to do in rust-lang/cargo#3815 is to allow running cargo during the moz.build processing, so that cargo can output a list of steps & a DAG of the rustc commands it is planning to invoke. Those commands could then be ingested into the larger gecko DAG and used by tup or another build system. So cargo tells what commands to run, but tup would actually invoke the individual rustc commands and be able to track dependencies on each invocation. Since the minimal rebuild in rust is a crate anyway, this maps well to the underlying language.

Long story short, I agree it would be ideal for tup to support unknown outputs in some way, but I don't think that will really help the cargo use-case.

@gittup
Copy link
Owner

gittup commented Aug 8, 2020

I finally merged this, and added the code to fix moving temporary directories in fuse. Sorry it took forever!

@gittup gittup closed this Aug 8, 2020
@layus
Copy link
Contributor Author

layus commented Aug 9, 2020

Nice ! Thanks :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants