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

Don't fail loading subprojects if subprojects_dir is in a subdirectory #2943

Merged
merged 4 commits into from
Feb 18, 2018
Merged

Conversation

ximion
Copy link
Contributor

@ximion ximion commented Jan 18, 2018

This resolves an error introduced in Meson 0.44.

If project B defines a subproject_dir of e.g. stuff/subprojects, and depends on a project A that gets pulled in via a wrapfile, which itself tries to compile a file defs.c that belongs to project A, Meson will fail to configure the project with the very helpful message:

Also couldn't find a fallback subproject in stuff/subprojects/a for the dependency A

Upon further investigation, it becomes clear that the real issue is Sandbox violation: Tried to grab file defs.c from a nested subproject. which is a wrong assumption from Meson, as defs.c clearly belongs to the right project.

This PR will make Meson display a nicer error message in these scenarios, as well as fix the actual sandbox issue.

The whole process might need a bit more work though - or I don't understand it fully yet ;-)
The check contains the following line:

if num_sps > 1:
            raise InterpreterException('Sandbox violation: Tried to grab file %s from a nested subproject.' % plain_filename)

Supposedly, this is protecting the user from a subproject trying to access a file from a different nested subproject. But the previous code that generates num_sps as far as I can see doesn't prevent that:

def evaluate_subproject_info(self, path_from_source_root, subproject_dirname):
        depth = 0
        subproj_name = ''
        segs = path_from_source_root.split(os.path.sep)
        while segs and segs[0] == subproject_dirname:
            depth += 1
            subproj_name = segs[1]
            segs = segs[2:]
        return (depth, subproj_name)

If the subproject A being loaded has overridden its subproject_dir to something that is not the exact same directory as the one from the main project, we will never get a depth (== num_sps) that is > 1.

The third check there though, seems to prevent most of the errors and makes a lot of sense:

if sproj_name != self.subproject_directory_name:
            raise InterpreterException('Sandbox violation: Tried to grab file %s from a different subproject.' % plain_filename)

If the subproject configuration is executed recursively (what I assume), this check will reliably prevent people fro tampering with subproject files.

So, tl;dr: This PR fixes the regression from Meson 0.44, but might not also resolve the original issue that led to the regression in the first place (unless I don't fully understand how the subprojects logic works, which is very likely ^^).

See also issue #2719 , CC: @jpakkane

@jpakkane
Copy link
Member

The appveyor test failure seems relevant.

@ximion
Copy link
Contributor Author

ximion commented Jan 29, 2018

@jpakkane Hmm... Is it really? I unfortunately have no idea on Windows things. To me it looks like Meson works as intended (the output is perfectly okay, since some subprojects are expected to fail / not be found), but then for some reason the VisualStudio solution fails to compile, and I don't see any actual reason for that in the logs. I briefly thought it would just grep for "error" in the logs, but that can't be the case as well.
The code in this patch also doesn't make assumptions on the OS used, so I don't know what is going on. In case someone knows and this is an issue with this patch, please point out the issue so I can attempt to fix it.

@ximion
Copy link
Contributor Author

ximion commented Feb 2, 2018

@jpakkane According to the AppVeyor matrix, the only difference between a working build and a successful one is a different MSVC version (MSVC2015 works, MSVC2017 fails) - therefore, I don't think the issue is related to this PR.
(And Travis is fine)

@jpakkane
Copy link
Member

jpakkane commented Feb 2, 2018

Retriggered build to see if it was sporadic.

@nirbheek
Copy link
Member

Is this still needed after #2890?

@nirbheek nirbheek added this to the 0.44.1 milestone Feb 11, 2018
@ximion
Copy link
Contributor Author

ximion commented Feb 11, 2018

@nirbheek Yes, this is a different bug of subprojects failing to configure if they are referenced by another project and are using a non-standard project directory. The previous bug was about subprojects failing to configure if they were using a subproject_dir in general.

@jpakkane The Travis build fails in one of the matrix builds while trying to fetch the container, so that one is definitely not related to this PR. The AppVeyer CI succeeds for all builds except the vs2017 backend on Windows (ninja works), and I have no idea why - so, if this is indeed related to this PR, I would need some help to figure out what is wrong, because I have no idea on how the vs2017 (or even Visual Studio) works here and also no way to test anything.
It's still weird to me though that this change would be a problem for the vs2017 backend - I rebased the patches on master, maybe that helps.

depth += 1
subproj_name = segs[1]
segs = segs[2:]
segs_spd = subproject_dirname.split(os.path.sep)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use pathlib.PurePath for all path manipulation. This will break if you use 'foo/bar' on Windows.

@ximion
Copy link
Contributor Author

ximion commented Feb 12, 2018

Hmm, now the vs2015 backend failed as well in AppVeyor, while previously only the vs2017 backend test failed - and the only change was using PurePath now, as suggested by Nirbheek.
Everything else looks fine.

@nirbheek
Copy link
Member

Strange, I can't see why only some of the backends would fail.

@nirbheek
Copy link
Member

Also, this needs a test or it'll get broken again.

@ximion
Copy link
Contributor Author

ximion commented Feb 12, 2018

Added test (fails with contrib/subprojects/beta/meson.build:3:0: ERROR: Sandbox violation: Tried to grab file b.c from a different subproject. on unpatched Meson, which is bogus, but succeeds with this PR merged).
The Windows error still doesn't make sense to me.

EDIT: Also rebased patches on master

Previously, Meson was showing a subproject being downloaded after later
claiming it doesn't exist.
This patch shows the actual error to clarify why the given subproject
can not be used.
@ximion
Copy link
Contributor Author

ximion commented Feb 14, 2018

I poked around a bit, and we are back to only one AppVeyor configuration failing, but this still does not make any sense to me.
All configurations work, except for the vs2017 backend with the msvc2017 compiler - the same config with the ninja backend works.
So, I guess really someone with Windows knowledge needs to take a look at this, or this is maybe not even related to this PR (master builds seem to succeed though, so there might be something wrong...)

@jpakkane
Copy link
Member

jpakkane commented Feb 17, 2018

The reason this fails is the following:

Starting with VS 2017 if the output of any command run by VS contains the word Error (edit, not actually true, it seems that the line needs to start with that) it will interpret that as a fatal error, even if the exit error code is zero.

If you change the error text from Error encountered to Problem encountered, the tests will pass. Dunno what the correct solution here is...

@ximion
Copy link
Contributor Author

ximion commented Feb 17, 2018

@jpakkane Haha, that's awesome ^^ - thank you for looking into this!
I will change "Error" to "Problem", because I don't see what else we could do here (unless VS has a flag that disables this behavior that we could toggle for the unit tests)
This resulted in a slightly ridiculous commit now - maybe someone has a better idea to solve this in the future...

Starting with VS 2017 if the output of any command run by VS contains
the word Error it will interpret that as a fatal error, even if the exit
error code is zero.
This messes up the unit tests on VS 2017, because we sometimes want to
deliberately ignore error messages.
Change "Error" to "Problem" to mitigate this issue until a more
permanent solution is found.
@ximion
Copy link
Contributor Author

ximion commented Feb 17, 2018

This did the trick :-)
Is there any ETA for Meson 0.44.1 or 0.45 by the way?

@jpakkane jpakkane merged commit 1841d53 into mesonbuild:master Feb 18, 2018
@nirbheek
Copy link
Member

Is there any ETA for Meson 0.44.1 or 0.45 by the way?

0.44.1 should happen in the next few days.

@ximion
Copy link
Contributor Author

ximion commented Feb 18, 2018

Nice, thanks! :-) - That will help a lot of projects running into these subproject issues on their CI.

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.

3 participants