652544 - dependsparser.py join code produces bogus results

Status: RESOLVED FIXED

(Tamarin Graveyard :: Build Config, defect, P1)

Description

[Tommy Reilly]

The fix for 632086 appears to when this was injected.   It works for some folks, don't know what makes it work/not-work yet.

Comment 1

[Tommy Reilly]

Attachment: Fix to use getcwd instead of originating file for relative_to_dir

Comment 2

[Tommy Reilly]

you must run make in the directory with the Makefile (which is required for my getcwd fix to be right):

treilly-MacBookPro:tamarin-redux treilly$ make -f foobar/Makefile 
foobar/Makefile:51: ../build/config.mk: No such file or directory
foobar/Makefile:52: ../manifest.mk: No such file or directory
foobar/Makefile:53: ../build/rules.mk: No such file or directory

So far so good....

Comment 3

[Tommy Reilly]

gcc produces the lines that match the input arg, ie:

gcc -E <snip>  ../other-licenses/zlib/infback.c  

and you get:

# 10 "../other-licenses/zlib/inffixed.h"

gcc -E <snip>  other-licenses/zlib/infback.c  

# 39 "other-licenses/zlib/zutil.h" 2

So we're invoking gcc with relative to CWD paths so that's what dependsparser should do.  I think its the right fix.

Comment 4

[Tommy Reilly]

Correct injection attribution: http://hg.mozilla.org/tamarin-redux/rev/b6682c9a0191 it was actually bug https://bugzilla.mozilla.org/show_bug.cgi?id=568237 and I checked that the selftest includes still resolve correctly with this fix.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 528115 [details] [diff] [review]
Fix to use getcwd instead of originating file for relative_to_dir

This patch in isolation is plausible.

I was tempted to R- it because: If we're getting rid of originating file then at the very least the documentation in the file needs to be updated, and if one is going to that trouble, then one might as well fix the calling sites to not pass the originating file either as part of standard code cleanup.

But I'm going to R+ it because: Since I'm the one who injected the bug in the first place, I'll post a patch covering those cleanup tasks.

Comment 6

[Tommy Reilly]

+1 I personally like to see fixes and cleanup posted for review and pushed separately (excepting a hard dependency between them of course).

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: code cleanup applicable post getcwd fix

This is meant to be layered on top of attachment 528115 [details] [diff] [review].

Comment 8

[Tommy Reilly]

While we're on the topic of code clean up, why does dependsparser write to stdout only to have the config.mk route it to /dev/null?

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #8)
> While we're on the topic of code clean up, why does dependsparser write to
> stdout only to have the config.mk route it to /dev/null?

Perhaps for making observations when debugging the tool itself?   I admit that is a pretty weak explanation.  I wouldn't object to commenting out the sys.stdout.write(line) invocation.

Comment 10

[Tommy Reilly]

I found it just got in the way when debugging using print and it seems wasteful to be writing everyline of of every preprocessed file to stdout only to pipe it to /dev/null in a shell command, I'll post another follow up clean up patch

Comment 11

[Tamarin Bot]

changeset: 6213:752763796d99
user:      Tommy Reilly <[e-mail]>
summary:   Bug 652544 - dependsparser should use cwd to do relative expansion, just like gcc does (r=fklockii,sr-pending=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/752763796d99

Comment 12

[Tommy Reilly]

Attachment: Remove copying stdin to stdout and then piping to /dev/null

Comment 13

[Tommy Reilly]

Attachment: Enhanced fix to support selftest style #line directives

Comment 14

[Tommy Reilly]

Attachment: cleanup a bit post fixes

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 528566 [details] [diff] [review]
Enhanced fix to support selftest style #line directives

ugh.  The "if not os.path.exists(path)" just sounds like a recipe for pain to me; in particular, the pain of debugging cases where there are false matches.

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #15)
> Comment on attachment 528566 [details] [diff] [review]
> Enhanced fix to support selftest style #line directives
> 
> ugh.  The "if not os.path.exists(path)" just sounds like a recipe for pain to
> me; in particular, the pain of debugging cases where there are false matches.

(I know the chances of a false match is rare, at least for the self-test.  But even so.)

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

I think I have an explanation for how this used to work and started breaking.

For Bug 568237, I authored code that handles absolute and relative paths differently, and attempted to resolve the relative paths.  That changeset was pushed back in June 2010.  At that time, the only source of relative paths in our build process were in the line directives inserted in the selftests; everything else had absolute paths.

For Bug 606989, I authored code that works around spaces in the absolute path of the build directory (on e.g. Windows) by changing the way we resolve the references to the topsrcdir; instead of resolving to an absolute path, it resolves to a relative path, relative to the build directory.  This changeset was pushed in March 2011 (recent!).

So now we're in a situation where problems in the fix for Bug 568237 are exposed, because many more paths show up as relative paths in the build process.  And that got us here.

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

Investigation has led me to agree with Tommy's conclusion that the selftests' #line directives are fundamentally at odds with gcc -E's #line directives.  So we should kill off the selftest #line directives.  Filed as bug 653220.

Comment 19

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 528567 [details] [diff] [review]
cleanup a bit post fixes

Looks great, I'll fold it into my other cleanup patch (the one from before your valiant attempt to continue supporting selftest #line's) and push.

Comment 20

[Tamarin Bot]

changeset: 6239:cf7c31a6c61b
user:      Felix S Klock II <[e-mail]>
summary:   Bug 652544: code cleanup (treilly r+'ed fklockii and fklockii r+'ed treilly).

http://hg.mozilla.org/tamarin-redux/rev/cf7c31a6c61b