Skip to content

Improve Bazel support#303

Open
Vertexwahn wants to merge 11 commits into
autodiff:mainfrom
Vertexwahn:bzlmod-support
Open

Improve Bazel support#303
Vertexwahn wants to merge 11 commits into
autodiff:mainfrom
Vertexwahn:bzlmod-support

Conversation

@Vertexwahn

@Vertexwahn Vertexwahn commented Nov 4, 2023

Copy link
Copy Markdown
  • 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

@allanleal allanleal left a comment

Copy link
Copy Markdown
Member

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 "".

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

Copy link
Copy Markdown
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 ""?

@Vertexwahn Vertexwahn Nov 6, 2023

Copy link
Copy Markdown
Author

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
Copy Markdown
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.

@allanleal allanleal Nov 8, 2023

Copy link
Copy Markdown
Member

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.

2 participants