dependsparser.py join code produces bogus results

RESOLVED FIXED in Q3 11 - Serrano

Status

P1
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: treilly, Assigned: pnkfelix)

Tracking

unspecified
Q3 11 - Serrano
x86
Mac OS X
Bug Flags:
flashplayer-injection +
flashplayer-qrb +

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 528115 [details] [diff] [review]
Fix to use getcwd instead of originating file for relative_to_dir
Attachment #528115 - Flags: feedback?(fklockii)
(Reporter)

Comment 2

8 years ago
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....
(Reporter)

Comment 3

8 years ago
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.
(Reporter)

Updated

8 years ago
Attachment #528115 - Flags: superreview?(edwsmith)
Attachment #528115 - Flags: review?(fklockii)
Attachment #528115 - Flags: feedback?(fklockii)
(Reporter)

Updated

8 years ago
Attachment #528115 - Attachment description: Tentative fix for sharing still researching → Fix to use getcwd instead of originating file for relative_to_dir

Updated

8 years ago
Flags: flashplayer-qrb+
Flags: flashplayer-injection+
(Reporter)

Comment 4

8 years ago
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 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.
Attachment #528115 - Flags: review?(fklockii) → review+
(Assignee)

Updated

8 years ago
See Also: → bug 568237
(Reporter)

Comment 6

8 years ago
+1 I personally like to see fixes and cleanup posted for review and pushed separately (excepting a hard dependency between them of course).
Created attachment 528305 [details] [diff] [review]
code cleanup applicable post getcwd fix

This is meant to be layered on top of attachment 528115 [details] [diff] [review].
Attachment #528305 - Flags: superreview?(edwsmith)
Attachment #528305 - Flags: review?(treilly)
(Reporter)

Comment 8

8 years ago
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?
(Reporter)

Updated

8 years ago
Attachment #528305 - Flags: review?(treilly) → review+
(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.
(Reporter)

Comment 10

8 years ago
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

8 years ago
changeset: 6213:752763796d99
user:      Tommy Reilly <treilly@adobe.com>
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
(Reporter)

Comment 12

8 years ago
Created attachment 528307 [details] [diff] [review]
Remove copying stdin to stdout and then piping to /dev/null
Attachment #528307 - Flags: superreview?(edwsmith)
Attachment #528307 - Flags: review?(fklockii)
(Reporter)

Comment 13

8 years ago
Created attachment 528566 [details] [diff] [review]
Enhanced fix to support selftest style #line directives
Attachment #528305 - Attachment is obsolete: true
Attachment #528305 - Flags: superreview?(edwsmith)
Attachment #528566 - Flags: review?
(Reporter)

Updated

8 years ago
Attachment #528566 - Flags: review? → review?(fklockii)
(Reporter)

Comment 14

8 years ago
Created attachment 528567 [details] [diff] [review]
cleanup a bit post fixes
Attachment #528307 - Attachment is obsolete: true
Attachment #528307 - Flags: superreview?(edwsmith)
Attachment #528307 - Flags: review?(fklockii)
Attachment #528567 - Flags: review?(fklockii)
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.
Attachment #528566 - Flags: review?(fklockii) → review-
(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.)

Updated

8 years ago
Attachment #528115 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Updated

8 years ago
Assignee: treilly → fklockii
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.
(Assignee)

Updated

8 years ago
See Also: → bug 606989
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.
(Assignee)

Updated

8 years ago
Depends on: 653220
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.
Attachment #528567 - Flags: review?(fklockii) → review+

Comment 20

8 years ago
changeset: 6239:cf7c31a6c61b
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 652544: code cleanup (treilly r+'ed fklockii and fklockii r+'ed treilly).

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

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.