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

Improve Bazel support #303

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Vertexwahn
Copy link

@Vertexwahn Vertexwahn commented Nov 4, 2023

  • Apply buildifier to Bazel build files
  • Add Bzlmod MODULE.bazel file
  • Rename from BUILD to BUILD.bazel
  • Switch angle brackets to double qoutes

@Vertexwahn Vertexwahn marked this pull request as draft November 4, 2023 16:20
@Vertexwahn Vertexwahn marked this pull request as ready for review November 4, 2023 16:50
@Vertexwahn Vertexwahn changed the title Apply buildifier to Bazel build files Improve Bazel support Nov 4, 2023
Copy link
Member

@allanleal allanleal left a comment

Choose a reason for hiding this comment

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

Hi @Vertexwahn , thanks for your contribution. Can we restrict this Pull Request to changes related to Bazel only? See other question on the need to change <> to "".

@@ -10,7 +10,7 @@ using namespace Eigen;

// autodiff includes
#define AUTODIFF_ENABLE_EIGEN_SUPPORT
#include <autodiff/reverse/var.hpp>
#include "autodiff/reverse/var.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Vertexwahn , why do we need to change from <> to ""?

Copy link
Author

@Vertexwahn Vertexwahn Nov 6, 2023

Choose a reason for hiding this comment

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

@allanleal I would use angle brackets only for system headers (e.g. <iostream>, <windows.h>) and double quotes for anything else (1, 2, 3) - this helps to get rid of copts = ["-I./"] which does not work when autodiff is used as a dependency

Copy link
Author

Choose a reason for hiding this comment

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

@allanleal Is there a reason why this should or cannot be changed to double quotes? Let me know if this is definitely no option for you.

Copy link
Member

@allanleal allanleal Nov 8, 2023

Choose a reason for hiding this comment

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

I understand your point. Note that the use of <> instead of "" can be seen in prominent C++ projects (e.g. Boost). Before making this change, we would need to ensure that the integration of autodiff in projects that depend on the CMake find_package command will not be broken. Would you be able to check that?

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.

None yet

2 participants