Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling for gcc directory search #11220

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EthanSteinberg
Copy link

This fixes a bug where meson fails to build on windows due to msys2 containing incorrect build paths. Most of the time Windows simply says that the directory doesn't exist, but in special cases (such as with BitLocker), an exception is thrown that blocks the build.

This catches the exception and converts it to a warning.

How these warnings look like in practice:

C:\envs\venv\Lib\site-packages\mesonbuild\compilers\mixins\gnu.py:497: UserWarning: Error processing directory [WinError -2144272384] This drive is locked by BitLocker Drive Encryption. You must unlock this drive from Control Panel: 'D:\a\_temp\msys64\ucrt64\x86_64-w64-mingw32\lib\x86_64-w64-mingw32\10.3.0'

This fixes a bug where meson fails to build on windows due to msys2 containing incorrect build paths.

This catches the exception and converts it to a warning.
@EthanSteinberg
Copy link
Author

Fixes #9127

@eli-schwartz
Copy link
Member

One of these days I should just purge the world (or at least Meson) of pathlib.Path, it's such a disaster. It's mind boggling that anyone thinks the correct way to handle "does this directory exist, yes or no" is to raise an exception when it cannot exist.

We could also just use os.path.exists(pobj).

@ilayn
Copy link

ilayn commented Dec 27, 2022

Why is this pathlib's fault? That's exactly what I want to see. Not silently ignoring problems but making noise when the question is not answerable. The folder might still be there but BitLocker nonsense didn't let the query go through.

For me, this is in fact a very good design decision instead of some shrugging and returning "No" only to find out that it is there when debugging and going nuts for hours.

The problem is how OS query design is done by the compiler folks returning all kinds of weird stuff not bothering to align on a standard.

@eli-schwartz
Copy link
Member

Considering how repeatedly buggy Meson has been every time any function anywhere uses concrete path methods, for example Meson crashing with uncaught exceptions when ~/ is not readable in a container but only if dependency('foo') is not installed and it tries to find it with cmake (because cmake dependencies use pathlib.Path().exists to check for some kind of package registry in your home directory!)...

I'm deeply, deeply unconvinced that the default behavior of "directory is not readable, or a very specific kind of WinError is reported when your RAM disk is produced by a highly specific company with buggy firmware" should be "raise an exception".

And I definitely don't think that all path handling code everywhere, no matter what, needs to attempt to catch multiple different exception hierarchies on every operation.

What's the meaningful difference here between "path doesn't exist" and "some bitlocker thing"? Isn't this already a path that legitimately doesn't exist? Seems to me like bitlocker shouldn't be involved with this msys2 misconfiguration, except that bitlocker by sheer coincidence happens to handle a drive by the same name.

If Windows didn't use drives, but used unix-style mountpoints, it wouldn't ever return an error like that because msys2's temporary paths would have an msys2 prefix. And the disk encryption software would have a bitlocker prefix.

If we wanted to warn about something, maybe we should check if a directory exists, silence all errors, and then warn if the directory doesn't exist. Don't just warn about particular subclasses of OS errors.

@ilayn
Copy link

ilayn commented Dec 27, 2022

Consider the alternative, you have folders that you are seeing with your own eyes, you are looking at your code and nothing makes sense because it can't find the folder. Then you start putting print statements everywhere only to find out the 4th path query blew up because some compiler insists on using *NIX practices on Windows. Because it's Microsoft's fault or some company made some mistake so the user should figure that out because it's user's fault to use the wrong tool and they should suffer. That takes things nowhere and it is wrong on many UX levels.

I'm deeply, deeply unconvinced that the default behavior of "directory is not readable, or a very specific kind of WinError is reported when your RAM disk is produced by a highly specific company with buggy firmware" should be "raise an exception".

In my opinion, it should. It is saying "I can't handle this particular case and I don't know what to do". That is by definition the exception that shouldn't have happened or how Python likes to work. The alternative is even worse; "I didn't manage to check but whatever YOLO it will explode somewhere else if this doesn't work out" which is the default mantra in many circles unfortunately. Most of these tools would not even be adopted (especially cmake) had they been written today with their extremely bad usability rep sheet. The reason why we want to use Meson IS avoiding make/cmake mind you :) so I share your disappointment fully. This is from yesterday scipy/scipy#17657 look at the actual problem about the path pkg-config is returning.

What's the meaningful difference here between "path doesn't exist" and "some bitlocker thing"?

The first one says, "I actually managed to check and the folder is not there". The second one says, "I can't tell, I was not allowed to peek into the drive" or whatever the reason was. One is answering the query, the second one is failing to query. If it was silent, there would be no way in hell we would notice this was happening until it blew up somewhere else.

If Windows didn't use drives, but used unix-style mountpoints, it wouldn't ever return an error like that because msys2's temporary paths would have an msys2 prefix. And the disk encryption software would have a bitlocker prefix.

I also wish Windows never was a thing but that ship has sailed decades ago I think. So I think we need to do a bit better in my opinion.

@eli-schwartz
Copy link
Member

And in this case:

  • if the directory doesn't exist it's a known bug in msys2
  • if the directory does exist, it's the wrong directory and probably has your unlocked documents from bitlocker
  • if the directory cannot be checked for, it's definitely not part of msys2, so it's the known bug in msys2 but also there's unrelated software doing something unrelated

So IMHO the problem is the known bug in msys2, and no matter why the directory doesn't exist, maybe we should warn about it? Why only warn about it when bitlocker is involved?

The alternative is even worse; "I didn't manage to check but whatever YOLO it will explode somewhere else if this doesn't work out" which is the default mantra in many circles unfortunately.

I think probably in practice, you always want to warn/error when an expected path problematically fails to exist, not when the lack of existence is because of a particular reason such as ramdisk firmware bugs causing it to report an exotic error instead of "DirectoryNotFound".

@EthanSteinberg
Copy link
Author

os.path.exists(pobj)

This would be bad because I think you do want to propagate that BitLocker error message to the user.

99% of the time, it will be nonsense but it's a really handy message in the scenarios where it applies.

@ilayn
Copy link

ilayn commented Dec 27, 2022

So IMHO the problem is the known bug in msys2, and no matter why the directory doesn't exist, maybe we should warn about it? Why only warn about it when bitlocker is involved?

Ah I understand what you mean. I think bitlocker or any other culprit is not that important, what I am trying to emphasize and probably not being quite clear about it, is that, when there is something out of the ordinary, it is perfectly fine to "halt and catch fire". The user will not (and should not) be blaming Meson for it because clearly this is weird circumstances. We can always choose to trust the user to fix things instead of trying to be smart about it if we supply sufficient information about it.

Obviously it is not my place to choose whether it is going to be a warning or an Exception. That's a design decision that I am not knowledgeable enough about Meson internals, to take. My emphasis is on the fact that "something happened" message surfaces all the way up to the user and that would give them some carrot to investigate further as opposed to trying to form the perfect environment that Meson expects with very few hints.

@EthanSteinberg
Copy link
Author

Gentle ping to maybe get this reviewed @eli-schwartz ?

I understand you want to make more complicated changes here, but you might want to consider merging this in the meantime just to unbreak builds on Windows machines with BitLocker.

@judemille
Copy link

I would like to have this merged, at the very least as a stop-gap until a better measure can be made. Let's not spend all our time bikeshedding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants