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

Status: RESOLVED INACTIVE

(Firefox Build System :: General, defect)

Description

[Pete Moore [:pmoore][:pete]]

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.

Comment 1

[Pete Moore [:pmoore][:pete]]

Attachment: bug1279027_gecko_v1.patch

Comment 2

[Pete Moore [:pmoore][:pete]]

Note, this can probably be done more nicely. Suggestions welcome! :-)

Comment 3

[Michael Shal [:mshal]]

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)

Comment 4

[Pete Moore [:pmoore][:pete]]

Thanks Mike - I'll do it like that. Will attach another patch. :-)

Comment 5

[Pete Moore [:pmoore][:pete]]

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.

Comment 6

[Pete Moore [:pmoore][:pete]]

(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.

Comment 7

[Pete Moore [:pmoore][:pete]]

(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.

Comment 8

[Pete Moore [:pmoore][:pete]]

Not actively working on this at the moment.