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

Move Au code into code/au subfolder #266

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Conversation

chiphogg
Copy link
Contributor

This replaces the project_symlinks approach we had been using for
CMake: it turns out that symlinks are fraught and error prone on
Windows. After this PR, CMake support on Windows is confirmed to work!

To make this work, we had to satisfy a number of challenging
constraints:

  1. The include path had to be "au/..." (e.g., "au/io.hh") for all
    files, on both bazel and CMake.

  2. The au folder that contained the actual code needed to live inside
    of a true subdirectory --- not the git repository root folder (as it
    has been since time immemorial), and not a symlink (as we had been
    using to get CMake support).

  3. The git repository root folder needed to also be the root of the
    bazel folder, because we use that for all of our bazel-backed tools,
    and because we want users to be able to run all bazel commands from
    the repo root.

I previously tried a variety of approaches here, including:

  • Having CMake create the symlink, instead of having it checked in (bad
    idea: you can't create symlinks in Windows without developer mode).

  • Making a code subfolder in the git repository root that had its own
    separate WORKSPACE file
    , and was included as a local bazel repo
    (terrible idea: we'd need two copies of all auxiliary bazel files, and
    making the single-file script work was nightmarishly complex --- I
    never even fully got there).

In the end, the approach that works is to use the includes attribute
of the cc_library rule. This is simple and direct: it's designed to
solve this exact problem. The only real downside is that we need to
prepend code/au to a lot of files in the BUILD.bazel for //au, and
also in a couple of places in the single file script.

Helps #215.

This replaces the `project_symlinks` approach we had been using for
CMake: it turns out that symlinks are fraught and error prone on
Windows.

To make this work, we had to satisfy a number of challenging
constraints:

1. The include path had to be `"au/..."` (e.g., `"au/io.hh"`) for all
   files, on both bazel and CMake.

2. The `au` folder that contained the actual code needed to live inside
   of a true subdirectory --- not the git repository root folder (as it
   has been since time immemorial), and not a symlink (as we had been
   using to get CMake support).

3. The git repository root folder needed to also be the root of the
   bazel folder, because we use that for all of our bazel-backed tools,
   and because we want users to be able to run all `bazel` commands from
   the repo root.

I previously tried a variety of approaches here, including:

- Having CMake create the symlink, instead of having it checked in (bad
  idea: you can't create symlinks in Windows without developer mode, and
  even after I enabled dev mode it didn't work).

- Making a `code` subfolder in the git repository root that had _its own
  separate `WORKSPACE` file_, and was included as a local bazel repo
  (terrible idea: we'd need two copies of all auxiliary bazel files, and
  making the single-file script work was nightmarishly complex --- I
  never even fully got there).

In the end, the approach that works is to use the `includes` attribute
of the `cc_library` rule.  This is simple and direct: it's designed to
solve this exact problem.  The only real downside is that we need to
prepend `code/au` to a lot of files in the `BUILD.bazel` for `//au`, and
also in a couple of places in the single file script.

Helps #215.
@chiphogg chiphogg added the release notes: ⚙️ repo PR affecting the way the repository works label Jul 20, 2024
@chiphogg chiphogg requested a review from geoffviola July 20, 2024 14:46
@@ -0,0 +1,505 @@
# Copyright 2024 Aurora Operations, Inc.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To review this file, "au/code/au/CMakeLists.txt", you can verify that it is identical to the contents of "au/CMakeLists.txt" on the parent branch. GitHub UI does not make it obvious, but we simply moved this file to this new location. (It doesn't show this because we had to retain a simpler version of the file at the old location.)

@@ -19,7 +19,8 @@ load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test")

cc_library(
name = "au",
hdrs = ["au.hh"],
hdrs = ["code/au/au.hh"],
includes = ["code"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about creating a bazel macro to handle this repetition, but I ended up deciding that direct representation might be easier to understand and maintain.

I realized I didn't need them before I added to them, but I forgot to
remove them earlier.
Will revert before landing.
Copy link
Contributor

@geoffviola geoffviola left a comment

Choose a reason for hiding this comment

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

The changes and rationale seem reasonable. I didn't get a chance to test it on Windows.

Code is a fine name. I've also seen src.

@chiphogg
Copy link
Contributor Author

I think src is typically used for the cc files, in setups where those are separate from the hh files, the latter of which tend to live in include. I thought code would be a good way to refer to both, given that our project layout doesn't separate them.

@chiphogg chiphogg merged commit e23614c into main Jul 22, 2024
11 checks passed
@chiphogg chiphogg deleted the chiphogg/code-subdir#215 branch July 22, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: ⚙️ repo PR affecting the way the repository works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants