Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

node variables + variants bugs #64

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

zjg commented May 28, 2012

Tried to get node-variables and variants both working on my big project, and found some bugs.

Contributor

zjg commented May 28, 2012

The following test case is currently broken as well:

#! /bin/sh -e
# tup - A file-based build system
#
# Copyright (C) 2009-2012  Mike Shal <marfey@gmail.com>
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License version 2 as
# published by the Free Software Foundation.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License along
# with this program; if not, write to the Free Software Foundation, Inc.,
# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

# Test that generated files can be referred to correctly from a Tupfile
# included through a node-variable, when using variants.
# (t2123 + variants)

. ./tup.sh
check_no_windows variant

tmkdir build
tmkdir sw
tmkdir lib

cat > Tuprules.tup << HERE
&lib_tupfile = lib/lib.tup
HERE

cat > lib/lib.tup << HERE
static_libs += \$(TUP_CWD)/lib.a
HERE

cat > lib/Tupfile << HERE
: foo.a |> cp %f %o |> lib.a
HERE

cat > sw/Tupfile << HERE
include_rules
include &(lib_tupfile)
: foreach \$(static_libs) |> cp %f %o |> %b.copy
HERE

tup touch Tuprules.tup lib/lib.tup lib/foo.a lib/Tupfile sw/Tupfile build/tup.config
update

tup_dep_exist lib lib.a build/sw 'cp ../../lib/lib.a lib.a.copy'

eotup

It fails with:
tup error: Explicitly named file 'lib.a' not found in subdir 4.

If I remove sw/Tupfile and do an update to create lib.a in the variant, and then restore sw/Tupfile, it then fails with:
tup error: Explicitly named file 'lib.a' is a ghost file, so it can't be used as an input.

I haven't looked too deeply into it yet, but I think that the included file is being parsed with the current directory set to the source dir, rather than the variant dir.

I'm not sure if a better fix would be to change the tupid that the node variables is storing (so that it points to the variant version and not the source version), or to just convert to the correct dir tupid when including through a node variable. I still need to spend some time with the variant code to get familiar with it.

Owner

gittup commented May 31, 2012

On Mon, May 28, 2012 at 2:08 PM, Doug Rosvick
reply@reply.github.com
wrote:

You can merge this Pull Request by running:

 git pull https://github.com/zjg/tup node-variables

Or you can view, comment on it, or merge it online at:

 #64

-- Commit Summary --

  • added t2123 (already worked) and t8067 (was broken: node-variables referring to non-generated files in variants)

-- File Changes --

M src/tup/parser.c (13)
A test/t2123-node-var-generated-file-from-include.sh (45)
A test/t8067-node-var-subdir.sh (37)

-- Patch Links --

 https://github.com/gittup/tup/pull/64.patch
 https://github.com/gittup/tup/pull/64.diff


Reply to this email directly or view it on GitHub:
#64

Merged - thanks!

-Mike

zjg added some commits Jun 2, 2012

Different way of implementing node vars: using relative paths from th…
…e root.

This avoids needing a bunch of logic to make node vars interact with variants correctly.
The paths generated when dereferencing a node var are slightly different, but still point to the same files.
Users of node vars shouldn't notice the change, unless they are saving the dereferenced path (which they shouldn't be doing).
Contributor

zjg commented Jun 2, 2012

I've re-implemented how node variables work behind the scenes, in order to make them work correctly with variants.

Instead of storing raw tupids, the node vars store the given path relative to the root of the tree. Thus the path is effectively an absolute path to tup, and referencing the file works correctly with variants (since it goes through all the same logic that normal file references goes through).

There is still a check in parser.c::set_variable to ensure that the referenced file exists (or is generated). I suppose this could be removed, but that changes the semantics of when node variables can be used, so I'm not sure if we want to.

Owner

gittup commented Jun 6, 2012

On Sat, Jun 2, 2012 at 10:59 AM, Doug Rosvick
reply@reply.github.com
wrote:

I've re-implemented how node variables work behind the scenes, in order to make the work correctly with variants.

Instead of storing raw tupids, the node vars store the given path relative to the root of the tree. Thus the path is effectively an absolute path to tup, and referencing the file works correctly with variants (since it goes through all the same logic that normal file references goes through).

There is still a check in parser.c::set_variable to ensure that the referenced file exists (or is generated). I suppose this could be removed, but that changes the semantics of when node variables can be used, so I'm not sure if we want to.


Reply to this email directly or view it on GitHub:
#64 (comment)

Hmm it seems weird to me that you end up with paths that go "outside"
the variant to the top of the tup hiearchy. Eg in t8067, rather than
./myLib.a it goes to ../myLib.a. Although it seems to work at the
moment, I would prefer if the file accesses go through the variant
paths and then the filesystem backend redirects to the source path as
necessary (eg: in the variant, "build" should be the top, not "." -
then fuse redirects accesses from "build" to "." for normal files).
What do you think about the attached patch? Basically it just ignores
variant directories for purposes of get_all_parent_tents, so whether
it is generated/normal or variant/not-a-variant, it pretends
everything is in the src dir. This passes all the test cases, but I
don't know if you had something else in mind that your change fixes.

To apply I reverted 9f1d7be and then applied the patch. If it looks
good to you, I guess you could either rebase and use this patch as a
commit instead of 9f1d or just revert that and add the patch at the
end.

All the other commits on your branch look good to me. Thanks for the help!

-Mike

Contributor

zjg commented Jun 6, 2012

(Did you forget to attach the patch?)

I was a bit unclear; by 'root of the tree' I meant the root of the variant, not all the way up to the .tup root.
This can be seen in t8068, where we have this layout:

.
|-- build
|   `-- tup.config
|-- lib
|   |-- foo.a
|   |-- lib.tup
|   `-- Tupfile
|-- sw
|   `-- Tupfile
`-- Tuprules.tup

And running tup upd --verbose:

[ tup ] [0.000s] Scanning filesystem...
[ tup ] [0.015s] Reading in new configuration/environment variables...
 1) new variant: build/tup.config
 [ ] 100%
[ tup ] [0.025s] Deleting 2 commands...
 1) rm: lib/lib.a
 2) rm: sw/lib.a.copy
 [  ] 100%
[ tup ] [0.027s] Parsing Tupfiles...
 1) [0.003s] build
 2) [0.004s] build/lib
 3) [0.006s] build/sw
 [   ] 100%
[ tup ] [0.041s] No files to delete.
[ tup ] [0.041s] Executing Commands...
 1) [0.007s] build/lib: cp foo.a lib.a
 2) [0.009s] build/sw: cp ../lib/lib.a lib.a.copy
 [  ] 100%
[ tup ] [0.060s] Updated.   

In command 2, the path only goes up to the root of the variant.

I think you might be looking at an old version of t8067; when I re-implemented node variables in 9f1d7be, I changed t8067 to get rid of the "../myLib.a" so that the command just asks for "myLib.a".

Anything I had in mind that my change fixes I should have put into a test case. Otherwise I wouldn't know when it works :P

Owner

gittup commented Jun 6, 2012

Oops, I was looking at t8067 after I had reverted 9f1d7be. I still think the patch is worth looking at to maintain the concept of the node-variable pointing to a specific ID rather than doing path manipulations, though. (As long as it doesn't break stuff). Some of the other paths also look strange with the path method, such as in t2118:

tup_dep_exist sw/app app.a sw/app 'cp ../../sw/app/app.a app.copy'

Rather than 'cp app.a app.copy'

It seems when I replied by email it ignored my patch, so here it is. I do think the error cleanups from 9f1d7be should still be in though.

diff --git a/src/tup/entry.c b/src/tup/entry.c
index 3c25c63..9ecb597 100644
--- a/src/tup/entry.c
+++ b/src/tup/entry.c
@@ -694,9 +694,22 @@ static int get_all_parent_tents(tupid_t tupid, struct tent_list_head *head)
{
struct tup_entry *tent;
struct tent_list *tlist;

  • struct variant *variant;

tent = tup_entry_get(tupid);

  • variant = tup_entry_variant(tent);
    while(tent) {
  •   if(!variant->root_variant) {
    
  •       /\* Pretend the variant dir isn't there. The parser
    
  •        \* handles whether or not the tent should be in the
    
  •        \* variant dir or the src dir - we just get the path as
    
  •        \* if the start and end tents are both in the src dir.
    
  •        */
    
  •       if(tent->dt == DOT_DT) {
    
  •           tent = tent->parent;
    
  •           continue;
    
  •       }
    
  •   }
    tlist = malloc(sizeof *tlist);
    if(!tlist) {
        perror("malloc");
    
    diff --git a/test/t8067-node-var-non-generated-file.sh b/test/t8067-node-var-non-generated-file.sh
    index a1cf632..57abb02 100755
    --- a/test/t8067-node-var-non-generated-file.sh
    +++ b/test/t8067-node-var-non-generated-file.sh
    @@ -32,6 +32,6 @@ HERE
    tup touch Tupfile build/tup.config myLib.a
    update

-tup_dep_exist . myLib.a build 'cp ../myLib.a myLib.a.copy'
+tup_dep_exist . myLib.a build 'cp myLib.a myLib.a.copy'

eotup

Owner

gittup commented Jun 6, 2012

Arg, that looks horrendous. Is there an easy way to do this in github? I'll send it directly to you as well.

Contributor

zjg commented Jun 6, 2012

In github, if you surround some text with three backticks (above & below), it is a "fenced code block", meaning that no other markup will be performed, and it will be a fixed-width font. See the "GitHub Flavoured Markdown" link for details (http://github.github.com/github-flavored-markdown/)

Skip over variant root tents when getting the relative path between t…
…wo tents.

This fixes node variables refering to generated files in variants, without needed to store paths in the node variables themselves.
Contributor

zjg commented Jun 6, 2012

I never liked the way I was storing paths for node variables... I also didn't think to look in get_relative_dir for the fix :) Thanks for the help.

I've renamed get_parent_tents since by skipping the variant root it didn't actually return the parent tents anymore, it returned the tents needed to generate a path to the given tupid (which is how it is being used anyways).

For the error message cleanup, I'll send that as a separate pull request, since it has nothing to do with node variables.

The only thing I know of that isn't working with node variables yet is when full-deps is on, you can get a node variable to refer to an absolute path, but only if you modify Tupfiles & update in a certain sequence. I have a test case for it (f664118), I just haven't looked into it yet.

Owner

gittup commented Jun 6, 2012

On Wed, Jun 6, 2012 at 10:58 AM, Doug Rosvick
reply@reply.github.com
wrote:

I never liked the way I was storing paths for node variables... I also didn't think to look in get_relative_dir for the fix :) Thanks for the help.

I've renamed get_parent_tents since by skipping the variant root it didn't actually return the parent tents anymore, it returned the tents needed to generate a path to the given tupid (which is how it is being used anyways).

For the error message cleanup, I'll send that as a separate pull request, since it has nothing to do with node variables.

The only thing I know of that isn't working with node variables yet is when full-deps is on, you can get a node variable to refer to an absolute path, but only if you modify Tupfiles & update in a certain sequence. I have a test case for it (f664118), I just haven't looked into it yet.

I had merged in some Windows changes before merging in this branch,
and I ran into some annoying conflicts since I had tried to fix some
of the tests to work there. It was easier for me to cherry-pick the
appropriate commits rather than try to get it to merge properly. Sorry
if that screws with anything on your end - you will probably want to
re-create your node-variables branch if you plan on making additional
changes on it. All of the tests and new functionality you added should
be in master now, though.

Regarding t2126, what does full-deps have to do with it? I seem to get
the same behavior even if full-deps is not enabled in the test (ie: I
don't get an error message trying to refer to the full-path node
variable).

Thanks!
-Mike

@gittup gittup closed this Sep 26, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment