Skip to content

Switching to using our own flatcc compiler library.#7468

Merged
benvanik merged 1 commit into
mainfrom
benvanik-eject
Oct 27, 2021
Merged

Switching to using our own flatcc compiler library.#7468
benvanik merged 1 commit into
mainfrom
benvanik-eject

Conversation

@benvanik

Copy link
Copy Markdown
Collaborator

We were already doing our own runtime library slicing, and we have to do our own compiler build for bazel, so this is consistent and lets us avoid including their cmake file (and the issues that creates).

@GMNGeoffrey GMNGeoffrey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lol almost less code

@benvanik benvanik disabled auto-merge October 27, 2021 03:13
@benvanik benvanik enabled auto-merge (squash) October 27, 2021 03:33
@benvanik benvanik merged commit 6874719 into main Oct 27, 2021
@benvanik benvanik deleted the benvanik-eject branch October 27, 2021 03:38
@powderluv

Copy link
Copy Markdown
Collaborator

This is awesome. I was tired of their damn CMake warnings I tried to submit a PR upstream dvidelabs/flatcc#217 ... one less warning :D

@GMNGeoffrey

Copy link
Copy Markdown
Contributor

Yeah this has bugged me. The latest weirdness was their CMake touching a file in the source tree on every configure: https://discord.com/channels/689900678990135345/689957613152239638/902723131939094618

@GMNGeoffrey GMNGeoffrey added the quality of life 😊 Nice things are nice; let's have some label Oct 27, 2021
@hanhanW hanhanW mentioned this pull request Oct 28, 2021
@mikkelfj

mikkelfj commented Jun 6, 2022

Copy link
Copy Markdown

@GMNGeoffrey

Yeah this has bugged me. The latest weirdness was their CMake touching a file in the source tree on every configure:

I can't follow that discord link, but are you referring to CMake in general at that version, or something in the flatcc configuration? I would also like to not have the source tree touched. (Author of flatcc speaking.)

In either case, flatcc was intended to be easily integrated in end users builds more than necessarily consuming the existing CMake project as is.

@GMNGeoffrey

Copy link
Copy Markdown
Contributor

Here's the place where it is touching the source tree: https://github.com/dvidelabs/flatcc/blob/62a7f88611af08c68be5485ce6d9c7e92474eab8/CMakeLists.txt#L135-L143

@mikkelfj

mikkelfj commented Jun 6, 2022

Copy link
Copy Markdown

Thanks for pointing this out, but this touches a file in the build directory, not in the source tree, and it is covered by .gitignore.

There is probably a more sane way to configure this, but the problem being solved here is that flatcc depends on its own output, and if the output somehow becomes invalid during development, it cannot bootstrap itself to fix the problem without removing the feature that depends on its own output (being able to read binary schema files in flatbuffer format).

There is actually another file scripts/build.cfg potentially being touched to remember the build backend (ninja or make) when using build scripts in the scripts folder, but if you use CMake directly this will not happen, and the file is also covered by .gitignore.

@GMNGeoffrey

Copy link
Copy Markdown
Contributor

Thanks for pointing this out, but this touches a file in the build directory, not in the source tree, and it is covered by .gitignore.

Only if the build directory is located within the source directory and called "build", which is not always the case. If you want to use the build directory, you should use PROJECT_BINARY_DIR instead of PROJECT_SOURCE_DIR and a hardcoded "build"

@mikkelfj

mikkelfj commented Jun 7, 2022

Copy link
Copy Markdown

OK, I removed the offensive files - they were only used to flag test scripts that needed to know, but they were largely obsolete anyway. All the shell script build and test drivers still make assumptions about build location, but CMake itself shouldn't, I agree.

@GMNGeoffrey GMNGeoffrey added the infrastructure Relating to build systems, CI, or testing label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup 🧹 infrastructure Relating to build systems, CI, or testing quality of life 😊 Nice things are nice; let's have some

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants