Page MenuHomePhabricator

[libc++] [test] Generate static_test_env on the fly
ClosedPublic

Authored by broadwaylamb on Apr 15 2020, 6:25 AM.

Details

Summary

Instead of storing static_test_env (with all the symlinks) in the repo, we create it on the fly to be cross-toolchain-friendly. The primary use case for this are Windows-hosted cross-toolchains. Windows doesn't really have a concept of symlinks. So, when the monorepo is cloned, those symlinks turn to ordinary text files. Previously, if we cross-compiled libc++ for some symlink-friendly system (e. g. Linux) and ran tests on the target system, some tests would fail. This patch makes them pass.

I'm not sure though about the need to create the RAII object in each test case that relies on static_test_env being present. Maybe a global would be better. Please let me know what you think.

Diff Detail

Event Timeline

broadwaylamb created this revision.Apr 15 2020, 6:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 6:25 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
broadwaylamb edited the summary of this revision. (Show Details)Apr 15 2020, 6:32 AM

Or maybe we'd rather generate the whole tree of static_test_env, not just symlinks?

ldionne requested changes to this revision.Apr 15 2020, 11:40 AM

I kind of think that we should do away with the static test env entirely, and always create it on the fly when we need it. It seems like this would be cleaner, and if we made .Root a member variable of static_test_env, it would be impossible to forget to create it. Right now, it's not really obvious how the static test env is created, and with this patch it's possible to forget to create it properly altogether.

My preference would be that static_test_env sets up the whole static test environment in the current directory, and that tests that need it create the environment as a local variable. We could also remove it from // FILE_DEPENDENCIES lines in the relevant tests. WDYT?

libcxx/test/support/filesystem_test_helper.h
46–53

I normally love comments, but I don't think the explanation part of why we're doing it this way now belongs in that comment. This is for the git history to tell instead.

57

range-based for loop?

61

What about the bad symlink -- won't it fail here?

This revision now requires changes to proceed.Apr 15 2020, 11:40 AM

Or maybe we'd rather generate the whole tree of static_test_env, not just symlinks?

I saw your comment after I had typed mine. But if that wasn't clear, that's basically what my comment meant.

EricWF added a subscriber: EricWF.Apr 16 2020, 10:48 AM

The entire point of the static test environment is that it's /static/.
Meaning it's checked in and not created on the fly.

broadwaylamb retitled this revision from [libc++] Generate symlinks in static_test_env on the fly to [libc++] [test] Generate static_test_env on the fly.
broadwaylamb edited the summary of this revision. (Show Details)

Generate all static_test_env on the fly, not just symlinks

The entire point of the static test environment is that it's /static/.
Meaning it's checked in and not created on the fly.

Do you have an idea to deal with the problem described on Windows? Would you prefer if we only created symlinks on the fly (like the first version of this patch does)?

@ldionne @EricWF am I getting it right that this patch is the wrong direction? How the described Windows issue could be resolved otherwise?

ldionne accepted this revision.May 4 2020, 9:54 AM

@ldionne @EricWF am I getting it right that this patch is the wrong direction? How the described Windows issue could be resolved otherwise?

I personally think it's the right direction. I believe Eric's point is that creating the static environment *statically* has the benefit that we know exactly what input our tests are running on, whereas we don't when we create the environment on the fly (e.g. there could be a bug in how we generate the env). However, given the difficulty with Windows, I'm OK with that.

Unless we can find another way to solve this problem, I believe we should move forward with this patch so as to enable Filesystem testing from a Windows host.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.current_path/current_path.pass.cpp
54–56

Just to confirm, you add the guard here because we change the current directory, but the directory is then removed so the test would fail if we didn't go back to the previous cwd at the end?

This revision is now accepted and ready to land.May 4 2020, 9:54 AM
broadwaylamb marked an inline comment as done.May 4 2020, 10:12 AM
broadwaylamb added inline comments.
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.current_path/current_path.pass.cpp
54–56

This test wouldn't fail, but other tests would. Otherwise yes, exactly as you said.

This revision was automatically updated to reflect the committed changes.
EricWF reopened this revision.May 5 2020, 6:52 PM

When I initially wrote the filesystem tests, I had divided them between "static" and "dynamic"; that is, tests which modified the target filesystem and those which did not.

I was concerned that some targets supported by libc++ wouldn't have the runtime dependencies for "dynamic" tests available. I didn't want these targets to have no filesystem tests,
I tried to write a minimal subset of tests which were "static".

I don't know that my initial concerned panned out, so I have reserved support for removing it if needed. That said, I don't understand how this patch makes these tests pass on Windows.
If Windows doesn't support symlinks, how does dynamically creating them make a difference?

Because this change unknowingly broke tests, I'm going to revert it.
We can recommit it after ensuring there are no other accidental changes.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_directory/is_directory.pass.cpp
65–66

This test is no longer correct.
I assume this change broke or changed the meaning of other tests as well.

If we're creating the static test env using a RAII guard,
that RAII guard should be the only way to name the members of the static environment
and the old StaticEnv::Foo variables should be removed.

This revision is now accepted and ready to land.May 5 2020, 6:52 PM
EricWF added a comment.May 5 2020, 6:58 PM

Unless I'm mistaken, this change is racy. Tests in different files are run in parallel.
One test could be tearing down the static environment while another in trying to construct it.

Update the diff to reflect the committed changes

broadwaylamb marked an inline comment as done.May 6 2020, 5:05 AM

I'm sorry, I've messed up. I first committed this as rG645ad5badbabdeca31de5c98ea8135c5a6e7d710, but then realized that I've committed the previous version of this patch. So I reverted it and recommitted as 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e.

That said, I don't understand how this patch makes these tests pass on Windows.
If Windows doesn't support symlinks, how does dynamically creating them make a difference?

This patch is not about making them pass on Windows, but rather about making them pass on Linux in the case when we cross-compile them on Windows.

Because this change unknowingly broke tests, I'm going to revert it.
We can recommit it after ensuring there are no other accidental changes.

Could you please point me to the test failures? I didn't receive any e-mails.

Unless I'm mistaken, this change is racy. Tests in different files are run in parallel.
One test could be tearing down the static environment while another in trying to construct it.

This is not the case as of 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e. Each RAII object creates a new temporary directory using mktemps (more precisely, it uses scoped_test_env under the hood).

Again, my apologies for messing up the diff here.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.is_directory/is_directory.pass.cpp
65–66

Now it should be fine.

I'm sorry, I've messed up. I first committed this as rG645ad5badbabdeca31de5c98ea8135c5a6e7d710, but then realized that I've committed the previous version of this patch. So I reverted it and recommitted as 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e.

That said, I don't understand how this patch makes these tests pass on Windows.
If Windows doesn't support symlinks, how does dynamically creating them make a difference?

This patch is not about making them pass on Windows, but rather about making them pass on Linux in the case when we cross-compile them on Windows.

In that case, would it not be sufficient to have the source tree checked out on the target system, and specify that location while compiling on the host?
I don't see why a change of this size is needed to solve that particular problem.

Because this change unknowingly broke tests, I'm going to revert it.
We can recommit it after ensuring there are no other accidental changes.

Could you please point me to the test failures? I didn't receive any e-mails.

The test I left that comment on was broken. It still passed, but it changed meaning.
It was checking that exists on a broken symlink returned false, but your change
turned a broken symlink into a file that didn't exist. So the test continued to pass,
but for the wrong reasons.

Unless I'm mistaken, this change is racy. Tests in different files are run in parallel.
One test could be tearing down the static environment while another in trying to construct it.

This is not the case as of 52cc8bac7780dbfb90dcc8cfe24696618eeaa06e. Each RAII object creates a new temporary directory using mktemps (more precisely, it uses scoped_test_env under the hood).

I don't see how that's possible since the static environment was still being referenced via the StaticEnv globals, but maybe I didn't look hard enough.

Again, my apologies for messing up the diff here.

EricWF requested changes to this revision.May 6 2020, 12:44 PM

If we don't want the static env, we should remove it entirely.
Meaning, we should convert each of these tests to use dynamic_test_env and setup the required files like the remainder of the tests do.

libcxx/test/support/filesystem_test_helper.h
306

I'm not OK with changing the tests working directory during the test.

This revision now requires changes to proceed.May 6 2020, 12:44 PM
EricWF added inline comments.May 6 2020, 12:56 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.current_path/current_path.pass.cpp
54–56

You are now using current_path in the setup for the current_path tests.
This is not OK. You cannot use the entity under test in said entities tests.

Please revert the patch.

@EricWF I understand your concern about using function foo() in the setup for testing function foo(). However, the flipside is that we have to reimplement e.g. current_path() ourselves in filesystem_helper.h, which entails basically copy-pasting it from the libc++ sources. And that's not much better IMO. Previously, we worked around that by using Python to do these things, but that assumes Python on the target, and we can't assume that.

If we had another way of solving the problem on Windows, we would not need this patch and everything would be much simpler. So let me ask again what I asked above:

Do you (or anyone else) have an idea to deal with the problem described on Windows? Would you prefer if we only created symlinks on the fly (like the first version of this patch does)?

For context, here's what the problem is:

Windows doesn't really have a concept of symlinks. So, when the monorepo is cloned, those symlinks turn to ordinary text files. Previously, if we cross-compiled libc++ for some symlink-friendly system (e. g. Linux) and ran tests on the target system, some tests would fail.

If we can solve this problem differently, I'm all for it. One possibility would be to store a tarball of the static environment in the source tree, and then automatically untar it only when on the target system -- this way Windows would never see any symlinks. Thoughts?

libcxx/test/support/filesystem_test_helper.h
306

Some tests already do this. This is just moving CwdGuard from canonical.pass.cpp to this header so it can be reused.

broadwaylamb marked 2 inline comments as done.EditedMay 6 2020, 1:37 PM

I've reverted the commit.

In that case, would it not be sufficient to have the source tree checked out on the target system, and specify that location while compiling on the host?
I don't see why a change of this size is needed to solve that particular problem.

Checking out the source tree on the target machine is not an option simply because the size of the monorepo checkout is close to the size of the remaining disk space on our board.

I don't see how that's possible since the static environment was still being referenced via the StaticEnv globals, but maybe I didn't look hard enough.

I've completely removed the StaticEnv namespace, so I'm pretty sure those globals are not referenced anymore.

If we don't want the static env, we should remove it entirely.
Meaning, we should convert each of these tests to use dynamic_test_env and setup the required files like the remainder of the tests do.

Would you be OK with this solution?

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.current_path/current_path.pass.cpp
54–56

Good point.

Would it be okay to use POSIX functions in the CWDGuard implementation instead?

libcxx/test/support/filesystem_test_helper.h
306

I'm not sure I understand what you mean. Some tests that test the current_path() function would inevitably change the working directory, it's just that we want to set it back so they don't affect other tests.

And if we're going to use dynamic_test_env, this will still be the case.

If we can solve this problem differently, I'm all for it. One possibility would be to store a tarball of the static environment in the source tree, and then automatically untar it only when on the target system -- this way Windows would never see any symlinks. Thoughts?

I like this!

smeenai added a subscriber: smeenai.May 6 2020, 1:44 PM

Sorry for jumping in without context, but wanted to chime in on one thing.

Windows doesn't really have a concept of symlinks. So, when the monorepo is cloned, those symlinks turn to ordinary text files. Previously, if we cross-compiled libc++ for some symlink-friendly system (e. g. Linux) and ran tests on the target system, some tests would fail.

This isn't right. Windows has supported symbolic links since Vista. The big limitation was that you needed to be in an elevated process to be able to create symbolic links. Windows 10 lifted that limitation in Developer Mode starting with the April 2017 release (https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/). If git for Windows isn't creating symbolic links properly, that's a git issue and not a Windows issue (although I recognize it doesn't make much difference for the end user). Some searching suggests that you should be able to configure Git on Windows to handle symlinks correctly (https://github.com/libgit2/libgit2/pull/4713, and https://github.community/t5/How-to-use-Git-and-GitHub/git-bash-symbolic-links-on-windows/m-p/11488#M3732 shows an option to enable symbolic links when running the setup).

broadwaylamb added a comment.EditedMay 6 2020, 1:55 PM

Sorry for jumping in without context, but wanted to chime in on one thing.

Windows doesn't really have a concept of symlinks. So, when the monorepo is cloned, those symlinks turn to ordinary text files. Previously, if we cross-compiled libc++ for some symlink-friendly system (e. g. Linux) and ran tests on the target system, some tests would fail.

This isn't right. Windows has supported symbolic links since Vista. The big limitation was that you needed to be in an elevated process to be able to create symbolic links. Windows 10 lifted that limitation in Developer Mode starting with the April 2017 release (https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/). If git for Windows isn't creating symbolic links properly, that's a git issue and not a Windows issue (although I recognize it doesn't make much difference for the end user). Some searching suggests that you should be able to configure Git on Windows to handle symlinks correctly (https://github.com/libgit2/libgit2/pull/4713, and https://github.community/t5/How-to-use-Git-and-GitHub/git-bash-symbolic-links-on-windows/m-p/11488#M3732 shows an option to enable symbolic links when running the setup).

I am aware of this, and I even had some success turning on support for symlinks on Windows. Which gives me the right to say that this process requires a little more involvement than it should :-)

However, things are even more complicated on the buildbot (which is the main purpose of this effort). Namely, there are some limitations of the buildbot platform that won't allow passing custom flags to Git when cloning the repo.

(But yeah, my wording could be more precise here.)

Sorry for jumping in without context, but wanted to chime in on one thing.

Windows doesn't really have a concept of symlinks. So, when the monorepo is cloned, those symlinks turn to ordinary text files. Previously, if we cross-compiled libc++ for some symlink-friendly system (e. g. Linux) and ran tests on the target system, some tests would fail.

This isn't right. Windows has supported symbolic links since Vista. The big limitation was that you needed to be in an elevated process to be able to create symbolic links. Windows 10 lifted that limitation in Developer Mode starting with the April 2017 release (https://blogs.windows.com/windowsdeveloper/2016/12/02/symlinks-windows-10/). If git for Windows isn't creating symbolic links properly, that's a git issue and not a Windows issue (although I recognize it doesn't make much difference for the end user). Some searching suggests that you should be able to configure Git on Windows to handle symlinks correctly (https://github.com/libgit2/libgit2/pull/4713, and https://github.community/t5/How-to-use-Git-and-GitHub/git-bash-symbolic-links-on-windows/m-p/11488#M3732 shows an option to enable symbolic links when running the setup).

I am aware of this, and I even had some success turning on support for symlinks on Windows. Which gives me the right to say that this process requires a little more involvement than it should :-)

However, things are even more complicated on the buildbot (which is the main purpose of this effort). Namely, there are some limitations of the buildbot platform that won't allow passing custom flags to Git when cloning the repo.

Buildbot's Git module allows you to pass in a config object, so I don't understand the problem.
http://docs.buildbot.net/0.8.9/manual/cfg-buildsteps.html#git

Are you using an even older version of Buildbot?
If so you can always insert the symlink config option by putting it in a .gitconfig file.

(But yeah, my wording could be more precise here.)

Buildbot's Git module allows you to pass in a config object, so I don't understand the problem.
http://docs.buildbot.net/0.8.9/manual/cfg-buildsteps.html#git

Are you using an even older version of Buildbot?
If so you can always insert the symlink config option by putting it in a .gitconfig file.

I didn't know about this, thanks.

However, the question is whether we want to depend on the presence of this filesystem feature on the host machine. I personally was surprised to see test failures caused by this, when everything else was in place. We could document that whoever wants to run libc++ tests in this setup will need to do some extra work setting up their environment and even re-cloning the whole monorepo (which takes some time). But I think it is too much to ask the user to do.

Besides, @danalbert has mentioned that Android would also benefit from this.

Reimplement CWDGuard using POSIX functions.

ldionne accepted this revision.May 22 2020, 12:52 PM

With the updated CWDGuard, I don't think we had any remaining major objections. Like we noted in our offline discussions, we would like to have more consistency between the static env and the dynamic env, but this patch seems like an OK stepping stone.

This revision was not accepted when it landed; it landed in state Needs Review.May 25 2020, 9:38 AM
This revision was automatically updated to reflect the committed changes.