Skip to content

Move header files to specific include directories#7477

Merged
sfc-gh-mpilman merged 14 commits into
apple:mainfrom
sfc-gh-mpilman:features/cmake-refactor-dirs
Jun 28, 2022
Merged

Move header files to specific include directories#7477
sfc-gh-mpilman merged 14 commits into
apple:mainfrom
sfc-gh-mpilman:features/cmake-refactor-dirs

Conversation

@sfc-gh-mpilman

@sfc-gh-mpilman sfc-gh-mpilman commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator

Currently we enforce include-modularization by copying header-files into the build directory. This is ugly and can cause confusions.

This PR addresses this by moving all header files into a special include directory.

Code-Reviewer Section

The general guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@fdb-windows-ci

Copy link
Copy Markdown
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

  • Commit ID: 41181cd
  • Duration 0:04:41
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 41181cd
  • Duration 0:04:49
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=OFF -D RUN_JAVA_INTEGRATION_TESTS=OFF -D BUILD_DOCUMENTATION=OFF -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 41181cd
  • Duration 0:05:01
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=ON -D RUN_JAVA_INTEGRATION_TESTS=ON -D BUILD_DOCUMENTATION=ON -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci

Copy link
Copy Markdown
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 7040cb9
  • Duration 0:05:02
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=ON -D RUN_JAVA_INTEGRATION_TESTS=ON -D BUILD_DOCUMENTATION=ON -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 7040cb9
  • Duration 0:05:03
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=OFF -D RUN_JAVA_INTEGRATION_TESTS=OFF -D BUILD_DOCUMENTATION=OFF -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

  • Commit ID: 7040cb9
  • Duration 0:05:16
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: ce30774
  • Duration 0:04:53
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=OFF -D RUN_JAVA_INTEGRATION_TESTS=OFF -D BUILD_DOCUMENTATION=OFF -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: ce30774
  • Duration 0:05:07
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=ON -D RUN_JAVA_INTEGRATION_TESTS=ON -D BUILD_DOCUMENTATION=ON -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 60bb544
  • Duration 0:09:30
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=ON -D RUN_JAVA_INTEGRATION_TESTS=ON -D BUILD_DOCUMENTATION=ON -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 60bb544
  • Duration 0:09:34
  • Result: ❌ FAILED
  • Error: Error while executing command: cmake -S . -B build_output -D USE_CCACHE=ON -D USE_WERROR=ON -D RocksDB_ROOT=/opt/rocksdb-6.27.3 -D RUN_JUNIT_TESTS=OFF -D RUN_JAVA_INTEGRATION_TESTS=OFF -D BUILD_DOCUMENTATION=OFF -G Ninja. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

  • Commit ID: 60bb544
  • Duration 0:09:51
  • Result: ❌ FAILED
  • Error: Error while executing command: ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -i ${HOME}/.ssh_key ec2-user@${MAC_EC2_HOST} /usr/local/bin/bash --login -c ./build_pr_macos.sh. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci

Copy link
Copy Markdown
Collaborator

Doxense CI Report for Windows 10

@@ -0,0 +1,4 @@
if(UNIX AND NOT APPLE)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

a nit: it is interesting we use APPLE as operating system, for instance here we compare it with UNIX; alternate would have been UNIX ANS NOT MAC.
No action needed in this PR, just sharing a thought.

@fdb-windows-ci

Copy link
Copy Markdown
Collaborator

Doxense CI Report for Windows 10

sfc-gh-ahusain
sfc-gh-ahusain previously approved these changes Jun 28, 2022

@sfc-gh-ahusain sfc-gh-ahusain left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great! Thanks for doing this, Markus.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 1bdbd89
  • Duration 0:29:13
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 20e7371
  • Duration 0:33:18
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 20e7371
  • Duration 0:33:18
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci

Copy link
Copy Markdown
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: fc2f695
  • Duration 0:14:11
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@sfc-gh-mpilman sfc-gh-mpilman marked this pull request as ready for review June 28, 2022 16:56
@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: fc2f695
  • Duration 0:15:11
  • Result: ❌ FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci

Copy link
Copy Markdown
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: a2ee14d
  • Duration 0:40:20
  • Result: ❌ FAILED
  • Error: Error while executing command: tar -czf "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION}/fdb_${FDB_VERSION}.tar.gz" --directory "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION} .. Reason: exit status 2
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

  • Commit ID: a2ee14d
  • Duration 0:40:49
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@sfc-gh-abeamon sfc-gh-abeamon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we also fix the casing of the header files that were moved (e.g. flow/serialize.h -> include/flow/Serialize.h)?

@sfc-gh-mpilman

Copy link
Copy Markdown
Collaborator Author

Can we also fix the casing of the header files that were moved (e.g. flow/serialize.h -> flow/include/Serialize.h)?

I would like to do this in a separate commit. This PR is stressful: every single commit that touches a header file creates a merge conflict for this PR.

@sfc-gh-abeamon

sfc-gh-abeamon commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator

Does that potentially cause issues? I know sometimes changing the case of a filename can break tools, and it might be easier to do it as part of the rename as a result. That's the main reason I brought it up as part of this activity.

@sfc-gh-mpilman

Copy link
Copy Markdown
Collaborator Author

I think after this PR it should cause fewer issues. This PR changes cmake so that it now looks for source files at configure time. So I wouldn't anticipate problems.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: a2ee14d
  • Duration 1:05:35
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: f9f5220
  • Duration 0:39:28
  • Result: ❌ FAILED
  • Error: Error while executing command: tar -czf "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION}/fdb_${FDB_VERSION}.tar.gz" --directory "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION} .. Reason: exit status 2
  • Build Logs (available for 30 days)

@sfc-gh-abeamon

Copy link
Copy Markdown
Collaborator

I suspect that this may cause problems for developers using case-insensitive file systems (e.g. on windows or macos) if they check out your changes before the follow-up PR to change case. I could be wrong though.

@sfc-gh-mpilman

Copy link
Copy Markdown
Collaborator Author

Yes, but I don't think doing it in this change will reduce the number of problems (it might be even worse since now you can have a changes serialize.h in flow/ and an unchanged Serialize.h in flow/include/flow).

My suggestion would be to not just rename the serialize.h to Serialize.h but instead rename it to something different, like Serialization.h.

@sfc-gh-abeamon

Copy link
Copy Markdown
Collaborator

I think that's a different type of problem, but we can just skip the renaming. It was just a nice to have cleanup operation that moving the files made easier.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: f9f5220
  • Duration 0:56:11
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@sfc-gh-mpilman sfc-gh-mpilman merged commit b236a2b into apple:main Jun 28, 2022
@sfc-gh-ljoswiak sfc-gh-ljoswiak deleted the features/cmake-refactor-dirs branch June 28, 2022 17:48
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.

6 participants