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

Always dereference symbolic links when copying files in ct #2740

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

gonzalobf
Copy link
Contributor

@gonzalobf gonzalobf commented Aug 25, 2022

I found that the patch I did for dereferencing symbolic links in common tests does not always work (I should have tried all the combinations).

The copy_bare_suites function only runs when the ct suite is placed in the root folder, but it doesn't run when it is placed in the apps/app_name/test/ folder. The first commit adds a test showing that this is the case.

In the second commit, I added copy_bare_suites to all the cases ofmaybe_inject_test_dir. I also forced dereferencing when copying the folders in rebar_prv_compile. I couldn't think in a case where this is not wanted and since rebar_prv_compile does run after copy_bare_suites, it always override the changes done by rebar3_prv_common_test.

The tests checks that the files and the symbolic files are copied
correctly to the _build folder.

In the case of symbolic links, the copy needs to dereference the
content. If not, the symbolic link can point to relative path that
doesn't exist in _build folder.

These tests try to prove that the feature added in [1] doesn't work
for all cases. When the `_SUITE_data` is inside an app test directory,
the content are copied without referencing.

[1] erlang#2731
@gonzalobf gonzalobf force-pushed the dereference-symbolic-links-compilation branch from e6a4532 to 3cdaf1d Compare January 27, 2023 08:49
@ferd
Copy link
Collaborator

ferd commented Jan 27, 2023

The copy_bare_suites function only runs when the ct suite is placed in the root folder, but it doesn't run when it is placed in the apps/app_name/test/ folder. The first commit adds a test showing that this is the case.

That sounds like exactly what it should be doing? A bare suite is a suite that is at the top-level test/ directory in an umbrella application -- and it therefore belongs to no app. If it's not at the top-level test/ directory in an umbrella, then it's not a bare suite, it's just a test suite.

The bare suite rules are weird because all compiled code needs to belong to something, and iirc the bare suite copying sort of creates a fake standalone directory to break away from that limitation, but normal test suites shouldn't suffer from that.

@gonzalobf
Copy link
Contributor Author

thanks @ferd, I totally misunderstand what was the intention of copy_bare_suites. Also, I just realised that I don't need to change rebar_prv_common_test.erl and by adding [{dereference, true}] to rebar_prv_compile is good enough to the tests to pass. Do you think I can still make this change or there are implications that would make it not wanted?

@gonzalobf gonzalobf force-pushed the dereference-symbolic-links-compilation branch from 3cdaf1d to 769ebe2 Compare January 30, 2023 11:18
Copy link
Collaborator

@ferd ferd left a comment

Choose a reason for hiding this comment

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

I think this is a proper fix, specifically because the copy/2 change you're making is used only for extra directories when compiling, (and not priv/ which could have different expectations for example) and that should be perfectly fine there. I would have been worried if we changed code in file utils, but this is narrowly scoped and probably fine.

I'll merge and we can see if any issue bubbles up from the community at this point.

@ferd ferd merged commit a68b50e into erlang:main Jan 30, 2023
@gonzalobf
Copy link
Contributor Author

Thanks! If anyone finds any issue, I'll be to happy to fix it :).

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.

2 participants