Use simple include path #591
Conversation
samples/CMakeLists.txt
Outdated
| @@ -45,6 +45,8 @@ endif() | |||
|
|
|||
| add_executable(amber ${AMBER_SOURCES}) | |||
| target_include_directories(amber PRIVATE "${CMAKE_BINARY_DIR}") | |||
| target_include_directories(amber PRIVATE | |||
| "${PROJECT_SOURCE_DIR}/third_party/lodepng") | |||
There was a problem hiding this comment.
Change this to just ${PROJECT_SOURCE_DIR}/third_party
samples/png.cc
Outdated
| @@ -21,7 +21,7 @@ | |||
|
|
|||
| #pragma clang diagnostic push | |||
| #pragma clang diagnostic ignored "-Wweak-vtables" | |||
| #include "third_party/lodepng/lodepng.h" | |||
| #include "lodepng.h" | |||
There was a problem hiding this comment.
This should be lodepng/lodepng.h. I'm pretty sure the linter is going to give an error with this being a directory-less include path.
samples/CMakeLists.txt
Outdated
| @@ -45,6 +45,8 @@ endif() | |||
|
|
|||
| add_executable(amber ${AMBER_SOURCES}) | |||
| target_include_directories(amber PRIVATE "${CMAKE_BINARY_DIR}") | |||
| target_include_directories(amber PRIVATE | |||
| "${PROJECT_SOURCE_DIR}/third_party/lodepng") | |||
samples/png.cc
Outdated
| @@ -21,7 +21,7 @@ | |||
|
|
|||
| #pragma clang diagnostic push | |||
| #pragma clang diagnostic ignored "-Wweak-vtables" | |||
| #include "third_party/lodepng/lodepng.h" | |||
| #include "lodepng.h" | |||
Right, but are we using lodepng in other places? Is this something that's blocking other work or something we may want in the future? |
|
I am testing Bazel build and I do not want to make lodepng as a submodule nor part of source code. I just let Bazel download lodepng from lodepng github directly. Bazel puts lodepng in other specific place if it is not the part of source code nor submodule. There is no way to change this specific location. |
This is similar to google#571. We do not want to use `third_party` as a part of include path.
|
Wait, I'm confused. You're writing a general Bazel script for Amber? Is this to be checked into the Amber repo? If so, it should continue to use the |
|
No. It is not a general Bazel script. It will be used only inside Google. Since other third parties of Amber used in Google has internal dependencies, it is hard to make them public. |
|
Downloading code in the build I don't think is allowed. Your better bet is to disable png support in google3 and instead just use the PPM code which is self contained. They this CL goes away and you create a CL to make lodepng optional. |
|
I tested the bazel script and it works fine. There are other projects inside Google (which is not part of google3) also did similar things. Actually I just mimic them. If Google inside does not need to update some third party code, many of projects let Bazel download those third parties from specific version of github commit. If you still doubt this, then I will follow what you mentioned. Actually, it is the same with my initial implementation. |
|
That surprises me. @dnovillo I always though code used in the build needed to be in the repo? Is it common to download thrid_party code and include it during the build? I think the simplest solution is to just disable PNG for internal and use the PPM output format which is fully contained inside Amber. Then you don't need lodepng at all. |
|
Supporting PNG is not a critical issue. Ok, let me disable PNG. |
As discussed in google#591, this CL sets lodepng as an optional library.
As discussed in #591, this CL sets lodepng as an optional library.
As discussed in google#591, this CL sets lodepng as an optional library.
This is similar to #571. We do not want to use
third_partyasa part of include path.