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

Copy the C header file in /usr/local/include #58

Open
dunglas opened this issue Oct 23, 2024 · 18 comments
Open

Copy the C header file in /usr/local/include #58

dunglas opened this issue Oct 23, 2024 · 18 comments

Comments

@dunglas
Copy link

dunglas commented Oct 23, 2024

Currently, it's a bit difficult for end users to install the C bindings. Couldn't we update the Cmake definition to automatically (or through an option) compile the C bindings, and copy the file to include in the system include path?

Also, this will help to add a formula for this lib in Homebrew.

@e-dant
Copy link
Owner

e-dant commented Oct 24, 2024

Yes, absolutely

@e-dant
Copy link
Owner

e-dant commented Oct 24, 2024

Added in 0.13.1, check out this: https://github.com/e-dant/watcher/actions/runs/11491524051/job/31984241215#step:11:1 for a dump of the artifacts from a typical cmake build on macOS, re. homebrew.

@e-dant
Copy link
Owner

e-dant commented Oct 24, 2024

Also -- The headers and the shared library are available for a variety of platforms/triples in the release artifacts, in case that's easier than cmake (or meson).

@dunglas
Copy link
Author

dunglas commented Oct 24, 2024

Thank you very much.

I'm looking for simple instructions for end users to install the library without building useless artifacts.

I used the typical cmake invocation:

cmake -S . -B build -DCMAKE_BUILD_TYPE=Release
cmake --build build/
sudo cmake --install build

This works, but a lot of files useless for production are compiled and installed, including sanitizers and snitch builds.

Setting -DWTR_WATCHER_BUILD_TEST=OFF -DWTR_WATCHER_BUILD_ASAN=OFF -DWTR_WATCHER_BUILD_MSAN=OFF -DWTR_WATCHER_BUILD_TSAN=OFF -DWTR_WATCHER_BUILD_UBSAN=OFF does not seem to help, the testing files are still installed.

The "standard" -DBUILD_TESTING=OFF is not defined.

Is there a way to build and install only production files? Could we add support for DBUILD_TESTING=OFF?

Thanks!

@e-dant
Copy link
Owner

e-dant commented Oct 24, 2024

Yes of course!

@e-dant
Copy link
Owner

e-dant commented Oct 24, 2024

How do these options look?

(Example snippet w/ no sanitizers, no tests)

$ cmake -S . -B out/tmp -DBUILD_TEST=OFF -DBUILD_SAN=OFF
...
-- wtr.hdr_watcher: Added (BUILD_HDR=ON)
-- watcher-c-hdr: Added (BUILD_HDR=ON)
-- watcher-c-shared: Added (BUILD_LIB=ON)
-- watcher-c-static: Added (BUILD_LIB=ON)
-- wtr.watcher: Skipped (BUILD_TEST=OFF)
-- wtr.watcher.asan: Skipped (BUILD_SAN=OFF)
-- wtr.watcher.msan: Skipped (BUILD_SAN=OFF)
-- wtr.watcher.tsan: Skipped (BUILD_SAN=OFF)
-- wtr.watcher.ubsan: Skipped (BUILD_SAN=OFF)
-- tw: Skipped (BUILD_TEST=OFF)
-- tw.asan: Skipped (BUILD_SAN=OFF)
-- tw.msan: Skipped (BUILD_SAN=OFF)
-- tw.tsan: Skipped (BUILD_SAN=OFF)
-- tw.ubsan: Skipped (BUILD_SAN=OFF)
-- wtr.test_watcher: Skipped (BUILD_TEST=OFF)
-- wtr.test_watcher.asan: Skipped (BUILD_SAN=OFF)
-- wtr.test_watcher.msan: Skipped (BUILD_SAN=OFF)
-- wtr.test_watcher.tsan: Skipped (BUILD_SAN=OFF)
-- wtr.test_watcher.ubsan: Skipped (BUILD_SAN=OFF)
-- portable-destructive-rename: Skipped (BUILD_TEST=OFF)
...

@e-dant
Copy link
Owner

e-dant commented Oct 24, 2024

Where the full set of options is:


option(BUILD_LIB   "Create a target for the watcher-c libraries"          ON)
option(BUILD_BIN   "Create a target for the CLI binaries"                 ON)
option(BUILD_HDR   "Create a target for the watcher headers"              ON)
option(BUILD_TEST  "Create a target for the test programs"                ON)
option(BUILD_SAN   "Mega-option to allow sanitizers"                      ON)
option(BUILD_ASAN  "Create a target for the address sanitizer"            ON)
option(BUILD_MSAN  "Create a target for the memory sanitizer"             ON)
option(BUILD_TSAN  "Create a target for the thread sanitizer"             ON)
option(BUILD_UBSAN "Create a target for the undefined behavior sanitizer" ON)

@dunglas
Copy link
Author

dunglas commented Oct 24, 2024

This looks good to me. What do you think about adding a BUILD_TESTING mega option that will set BUILD_TEST and BUILD_SAN to off? This option looks pretty standard and is added by default by Homebrew.

Thank you very much for your work on this!!

@e-dant
Copy link
Owner

e-dant commented Oct 26, 2024

Fixed in 55e6b15, I'll cut a release if #59 is resolvable

@e-dant
Copy link
Owner

e-dant commented Oct 26, 2024

Well... Let me fixup some build errors first...

@dunglas
Copy link
Author

dunglas commented Oct 27, 2024

I confirm that the next branch works as expected, thank you!

Another related thing: is it desirable/needable to install the wtr.watcher and tw binary in the system path? At least, maybe the tw binary should be renamed for something more explictiy like tiny_watcher?

@e-dant
Copy link
Owner

e-dant commented Oct 27, 2024

Another related thing: is it desirable/needable to install the wtr.watcher and tw binary in the system path? At least, maybe the tw binary should be renamed for something more explictiy like tiny_watcher?

Yeah, great question, and one which I've asked myself a lot.

The wtr.watcher binary started as an example of the library, but quickly turned into something that I used a lot. Brewing up quick scripts that

  • reload files
  • notify me of suspicious changes
  • undo changes on files that I don't want to be modified

Are examples of things I have running on my system.

I use wtr.watcher for that, and so it's useful for me personally to have it as an installation candidate.

tw is has a somewhat similar story; started as an example, but grew into something that I actually use. In this case, I find the output more human readable, so I tend to use it for interactive sessions.


You might notice a lot of "personally" or "for me" phrases up there -- that's really what I wonder about -- are these programs actually useful for other people? What do people use this project for; libraries or programs?

I don't know.


I started thinking about this one much more when I created examples in some of the other languages we support (Rust/Node.js/Python/C) which all do similar things.

What makes one of those programs less important than the original C++ programs? Which should we prefer, if any?


Your thoughts are definitely appreciated here.

@e-dant
Copy link
Owner

e-dant commented Oct 27, 2024

For the naming, I've been hesitant to change things in case there are people relying on them as-is. (But I think we can/should do it.)

However, if we change the binary names, I'd also like to change wtr.watcher (nearly nothing else in the universe has a . in a program name, and it was a mistake)

@LineGM
Copy link

LineGM commented Dec 11, 2024

This looks good to me. What do you think about adding a BUILD_TESTING mega option that will set BUILD_TEST and BUILD_SAN to off? This option looks pretty standard and is added by default by Homebrew.

In CMake there is already a BUILD_TESTING variable (https://cmake.org/cmake/help/git-stage/variable/BUILD_TESTING.html)
I ran into a problem when I used this project as a dependency (using FetchContent). I didn't want to build tests for this dependency and set this option to OFF, but this also disables tests in my project.
Maybe I'm missing something and it's possible to select an option for a specific CMake project?

@e-dant
Copy link
Owner

e-dant commented Dec 11, 2024

I am unfortunately not a CMake expert, and I'm not sure the best way to help.

However, I'm more than happy to change things in ways that make our CMake file more friendly to fetchcontent for this.

Any thoughts about the best thing to do here?

@LineGM
Copy link

LineGM commented Dec 13, 2024

I have found a workaround specifically for my project. The only thing that does not allow my project to build at once is cmake_minimum_required(VERSION 3.9). I have to change it manually to 3.10 :)
@e-dant maybe bump CMake version to 3.10, because in 3.31 anything < 3.10 is deprecated.
https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html#policy-settings

@dunglas
Copy link
Author

dunglas commented Dec 13, 2024

For the record, I opened a PR to add watcher to Homebrew: Homebrew/homebrew-core#201096

@e-dant
Copy link
Owner

e-dant commented Dec 14, 2024

For the record, I opened a PR to add watcher to Homebrew: Homebrew/homebrew-core#201096

This is so awesome!

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

No branches or pull requests

3 participants