python/mozbuild/mozbuild/frontend/reader.py should log non-compliant paths when throwing an error

NEW
Assigned to

Status

2 years ago
8 months ago

People

(Reporter: pmoore, Assigned: pmoore)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
reader.py can exit with an error that a mozconfig does not sit inside an allowed path, but does not report the location of the mozconfig that is deemed to sit outside the allowed path, and what the allowed path was.

Ideally when an error occurs due to an assertion failing, the data that was used to check the assertion should be included with the error message, thus making it possible to see why the assertion failed, not only that it failed.

In the recent case that impacted the windows taskcluster builds, it was due to the lexical check not considering that c:\..... could be a subdirectory of C:\..... due to the c: vs C:. This has been resolved separately by changing paths used in command arguments and environment variables, but the improved error message would have been useful and saved time.

Soft block only on 1244750 as it would be nice to land it at the same time, but not absolutely required.
(Assignee)

Comment 1

2 years ago
Created attachment 8761559 [details] [diff] [review]
bug1279027_gecko_v1.patch
Assignee: nobody → pmoore
Attachment #8761559 - Flags: review?(mshal)
(Assignee)

Comment 2

2 years ago
Note, this can probably be done more nicely. Suggestions welcome! :-)
(Assignee)

Updated

2 years ago
See Also: → bug 1266123
Comment on attachment 8761559 [details] [diff] [review]
bug1279027_gecko_v1.patch

This seems to split the error message into two different locations. We already get the path of the offending file in a message like:

"""
The error occurred while processing the following file:
 
    /home/mshal/mozilla-central-git/moz.build
 
The underlying problem is an illegal file access. This is likely due to trying to access a file outside of the top source directory.
 
The path whose access was denied is:
 
    /home/mshal/foo.mozbuild
"""

I think it might make sense to add what the topsrcdir is to this message, since that is I believe what was missing to make the c:\ vs. C:\ error more apparent. Maybe this could be:

    The underlying problem is an illegal file access. This is likely due to 
    trying to access a file outside of the top source directory (c:\worker\blah)
Attachment #8761559 - Flags: review?(mshal) → feedback+
(Assignee)

Comment 4

2 years ago
Thanks Mike - I'll do it like that. Will attach another patch. :-)
(Assignee)

Comment 5

2 years ago
OK, I've had a look, but I don't see an easy way to access the top source directory at that point in the code...

Error message is here:

https://dxr.mozilla.org/mozilla-central/rev/249d57fa78a0522f63a5b1ed1fdda55f383156e0/python/mozbuild/mozbuild/frontend/reader.py#705

Exception is raised here:
https://dxr.mozilla.org/mozilla-central/rev/249d57fa78a0522f63a5b1ed1fdda55f383156e0/python/mozbuild/mozbuild/frontend/reader.py#1066

Somehow the topsrcdir needs to be passed through into the exception, but I couldn't get my head around the exception classes.
(Assignee)

Comment 6

2 years ago
(In reply to Pete Moore [:pmoore][:pete] from comment #5)

> Exception is raised here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 249d57fa78a0522f63a5b1ed1fdda55f383156e0/python/mozbuild/mozbuild/frontend/
> reader.py#1066

Well really it is raised here:
https://dxr.mozilla.org/mozilla-central/rev/249d57fa78a0522f63a5b1ed1fdda55f383156e0/python/mozbuild/mozbuild/frontend/reader.py#1207

The previous link is where it is caught and passed up the stack.
(Assignee)

Comment 7

2 years ago
(In reply to Pete Moore [:pmoore][:pete] from comment #0)

> Soft block only on 1244750 as it would be nice to land it at the same time,
> but not absolutely required.

Removing the soft block, as it makes sense to land without it.
No longer blocks: 1244750

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.