fix: Remove cmake cache files after copying to container build directory #2325

Tha14 · 2022-07-04T20:31:52Z

This is a change to the build script of the docker container for cross compiling toxcore for windows. I use the same repository cloned dir when I am building for both windows and linux and what tends to happen is CMakeCache.txt and CMakeFiles dir get copied to the build directory inside the container image which results in an error when cmake runs:

CMake Error: The current CMakeCache.txt directory /tmp/toxcore/CMakeCache.txt is different than the directory /root/Desktop/c-toxcore where CMakeCache.txt was created. This may result in binaries being created in the wrong place. If you are not sure, reedit the CMakeCache.txt

To avoid having this problem I have added two rm commands to remove the files that cause the issue.

This change is

codecov · 2022-07-04T20:43:43Z

Codecov Report

Merging #2325 (e0b00d3) into master (7dfa935) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2325      +/-   ##
==========================================
+ Coverage   78.06%   78.08%   +0.01%     
==========================================
  Files         140      140              
  Lines       31053    31053              
==========================================
+ Hits        24243    24247       +4     
+ Misses       6810     6806       -4

Impacted Files	Coverage Δ
toxcore/onion.c	`85.11% <0.00%> (-0.85%)`	⬇️
toxcore/onion_announce.c	`85.44% <0.00%> (-0.38%)`	⬇️
toxcore/onion_client.c	`89.76% <0.00%> (-0.12%)`	⬇️
toxcore/DHT.c	`85.08% <0.00%> (-0.08%)`	⬇️
toxav/toxav.c	`69.23% <0.00%> (ø)`
toxcore/net_crypto.c	`87.36% <0.00%> (+0.14%)`	⬆️
toxcore/group_chats.c	`69.16% <0.00%> (+0.16%)`	⬆️
toxcore/TCP_client.c	`83.60% <0.00%> (+0.23%)`	⬆️
toxcore/ping.c	`86.41% <0.00%> (+0.61%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Tha14 · 2022-09-27T22:10:30Z

@robinlinden Hey, could you review this when possible? Thanks

Green-Sky

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained

Green-Sky

Reviewable status: 0 of 1 approvals obtained

nurupo

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @Tha14)

other/docker/windows/build_toxcore.sh line 74 at r1 (raw file):

    -DCMAKE_EXE_LINKER_FLAGS="$CMAKE_EXE_LINKER_FLAGS -fstack-protector" \
    -DCMAKE_SHARED_LINKER_FLAGS="$CMAKE_SHARED_LINKER_FLAGS" \
    "$EXTRA_CMAKE_FLAGS" \

Please remove the quotes, they are undesirable. You are essentially forcing $EXTRA_CMAKE_FLAGS to be passed to cmake as a single flag, where we want to allow it to contain multiple flags.

nurupo · 2022-10-06T15:48:25Z

This PR doesn't make sense. cmake gets called with /tmp/toxcore/build being the working directory, but /tmp/toxcore/CMakeCache.txt you are removing is outside of that directory, it shouldn't affect cmake. Did you mean to remove /tmp/toxcore/build/CMakeCache.txt instead?

other/docker/windows/build_toxcore.sh

Tha14

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

other/docker/windows/build_toxcore.sh line 74 at r1 (raw file):

Previously, nurupo wrote…

Please remove the quotes, they are undesirable. You are essentially forcing $EXTRA_CMAKE_FLAGS to be passed to cmake as a single flag, where we want to allow it to contain multiple flags.

Done.

nurupo · 2022-10-06T18:07:44Z

This PR doesn't make sense. cmake gets called with /tmp/toxcore/build being the working directory, but /tmp/toxcore/CMakeCache.txt you are removing is outside of that directory, it shouldn't affect cmake.

Ok, I'm able to reproduce this. This is weird, I expected cmake to look for CMakeCache.txt only inside the build dir, but apparently it's also looking for it in the source directory. What's weirder is that if it doesn't error on it, it just ignores it, e.g. I had CMakeCache.txt contain only BUILD_TOXAV:BOOL=OFF, yet cmake built toxav.

This seems to be more of an user error than a build script bug. The user made an in-tree cmake build and littered the source directory with cmake files. This is why it's recommended to use out-of-tree builds to avoid such issues.

I'm for merging this, but Tha_14 should really consider doing out-of-tree builds.

nurupo

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained

nurupo · 2022-10-07T14:26:27Z

Ok, I found the explanation of this weird behavior.

See the first two commands in: https://cmake.org/cmake/help/v3.24/manual/cmake.1.html?highlight=cmakecache%20txt#generate-a-project-buildsystem

cmake [<options>] <path-to-source>

Uses the current working directory as the build tree, and as the source tree. The specified path may be absolute or relative to the current working directory. The source tree must contain a CMakeLists.txt file and must not contain a CMakeCache.txt file because the latter identifies an existing build tree.

cmake [<options>] <path-to-existing-build>

Uses as the build tree, and loads the path to the source tree from its CMakeCache.txt file, which must have already been generated by a previous run of CMake. The specified path may be absolute or relative to the current working directory.

Essentially, because you had CMakeCache.txt file in the directory we specify when calling cmake, cmake treated that directory not as a source directory, but as a build directory, looking up the path to the source directory from the CMakeCache.txt file, and realizing that such path doesn't exist, resulting in the error you are seeing.

This explains why it was picking up the CMakeCache.txt outside the current directory when I didn't expect it to. With this new information, this looks to be more of a build script bug than a user error as I have mentioned earlier. A proper fix would be to change the last cmake argument from .. to -S .., to explicitly tell cmake that that's a source directory.

nurupo

I guess I should have commented via Reviewable so that I could do "Request changes".

Reviewable status: 1 change requests, 0 of 1 approvals obtained

CLAassistant · 2022-10-07T14:42:35Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

nurupo · 2022-10-07T15:02:57Z

That fixed the reproduced error for me. @Tha14 does it work for you?

Tha14

@nurupo Yes, I reproduced the mistake I did the first time and it fixes it.

Reviewable status: 1 change requests, 0 of 1 approvals obtained

nurupo

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained

nurupo · 2022-10-07T15:29:00Z

Restyled is a required check, huh. It's a false positive in this case, we don't want that variable quoted, we want word splitting in here. I could add shellcheck disable=SC2086 to make shellcheck ignore it, but there are other variables in that line that should be quoted and we would want to catch them being unquoted in the future...

cmake treats the provided path differently depending on whether it contains a CMakeCache.txt or not. If it doesn't contain it -- it's treated as a path to the source tree, if it does -- as a path to the build tree. We want it to be treated as a source tree path, but if a user has CMakeCache.txt in that directory, e.g. from a previous in-tree build the user has done, cmake will treat it as a build tree instead, which might lead to unexpected results (improperly configured build) or an error, with the latter being more likely considering we are building inside a container and the host paths specified in the user-generated CMakeCache.txt likely don't exist in there.

nurupo · 2022-10-07T16:04:48Z

Yes! At last, the final boss Restyled is defeated.

Having to add this totally unnecessary array just to work-around the linter check is kind of crazy though.

nurupo · 2022-10-07T16:18:38Z

Now the AppVeyor build failed:

Build execution time has reached the maximum allowed time for your plan (10 minutes).

It was about to run the last autotest and finish...

auto-add-label bot added the bug Bug fix for the user, not a fix to a build script label Jul 4, 2022

restyled-io bot mentioned this pull request Jul 4, 2022

Restyle fix: Remove cmake cache files after copying to container build directory #2326

Closed

Tha14 marked this pull request as ready for review July 4, 2022 20:47

JFreegman added this to the v0.2.19 milestone Sep 27, 2022

Green-Sky reviewed Oct 5, 2022

View reviewed changes

pull-request-attention bot assigned Tha14 Oct 5, 2022

Green-Sky reviewed Oct 5, 2022

View reviewed changes

nurupo requested changes Oct 6, 2022

View reviewed changes

Tha14 commented Oct 6, 2022

View reviewed changes

other/docker/windows/build_toxcore.sh Outdated Show resolved Hide resolved

restyled-io bot mentioned this pull request Oct 6, 2022

Restyle fix: Remove cmake cache files after copying to container build directory #2342

Closed

Tha14 commented Oct 6, 2022

View reviewed changes

nurupo approved these changes Oct 6, 2022

View reviewed changes

nurupo requested changes Oct 7, 2022

View reviewed changes

restyled-io bot mentioned this pull request Oct 7, 2022

Restyle fix: Remove cmake cache files after copying to container build directory #2344

Closed

Tha14 commented Oct 7, 2022

View reviewed changes

restyled-io bot mentioned this pull request Oct 7, 2022

Restyle fix: Remove cmake cache files after copying to container build directory #2345

Closed

nurupo approved these changes Oct 7, 2022

View reviewed changes

restyled-io bot mentioned this pull request Oct 7, 2022

Restyle fix: Remove cmake cache files after copying to container build directory #2346

Closed

Make Restyled/shellharden happy

e0b00d3

nurupo merged commit e0b00d3 into TokTok:master Oct 7, 2022

Tha14 deleted the fix_docker_windows_copying_cache_files branch October 11, 2022 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove cmake cache files after copying to container build directory #2325

fix: Remove cmake cache files after copying to container build directory #2325

Tha14 commented Jul 4, 2022 •

edited

Loading

codecov bot commented Jul 4, 2022 •

edited

Loading

Tha14 commented Sep 27, 2022

Green-Sky left a comment

Green-Sky left a comment

nurupo left a comment

nurupo commented Oct 6, 2022

Tha14 left a comment

nurupo commented Oct 6, 2022

nurupo left a comment

nurupo commented Oct 7, 2022 •

edited

Loading

nurupo left a comment

CLAassistant commented Oct 7, 2022 •

edited

Loading

nurupo commented Oct 7, 2022

Tha14 left a comment

nurupo left a comment

nurupo commented Oct 7, 2022

nurupo commented Oct 7, 2022

nurupo commented Oct 7, 2022

fix: Remove cmake cache files after copying to container build directory #2325

fix: Remove cmake cache files after copying to container build directory #2325

Conversation

Tha14 commented Jul 4, 2022 • edited Loading

codecov bot commented Jul 4, 2022 • edited Loading

Codecov Report

Tha14 commented Sep 27, 2022

Green-Sky left a comment

Choose a reason for hiding this comment

Green-Sky left a comment

Choose a reason for hiding this comment

nurupo left a comment

Choose a reason for hiding this comment

nurupo commented Oct 6, 2022

Tha14 left a comment

Choose a reason for hiding this comment

nurupo commented Oct 6, 2022

nurupo left a comment

Choose a reason for hiding this comment

nurupo commented Oct 7, 2022 • edited Loading

nurupo left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 7, 2022 • edited Loading

nurupo commented Oct 7, 2022

Tha14 left a comment

Choose a reason for hiding this comment

nurupo left a comment

Choose a reason for hiding this comment

nurupo commented Oct 7, 2022

nurupo commented Oct 7, 2022

nurupo commented Oct 7, 2022

Tha14 commented Jul 4, 2022 •

edited

Loading

codecov bot commented Jul 4, 2022 •

edited

Loading

nurupo commented Oct 7, 2022 •

edited

Loading

CLAassistant commented Oct 7, 2022 •

edited

Loading