-
Notifications
You must be signed in to change notification settings - Fork 189
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
fix: add --forward flag to PATCH_EXECUTABLE command #572
Conversation
This seems reasonable, but what happens if the patch is itself bad? In my testing and development, I don't think this tried to re-apply. I wonder if that's because I was using SHA256 summing? |
Not ideal at all. |
@ScottBailey The project where I encounter this issue is open: https://github.com/simdjson/experimental_json_builder/blob/main/benchmarks/CMakeLists.txt |
https://github.com/simdjson/experimental_json_builder/blob/main/benchmarks/CMakeLists.txt This is an interesting issue. I do NOT have this problem. What is your dev environment? What's different between our systems? I am Debian Linux with cmake 3.30.0-rc3. Granted, I couldn't compile the project. But I don't think that should matter. Do you have a cache path set? I do. |
I do not think it matters, but we do provide detailed instructions. I reproduce it here for your convenience. (It is in our README file.) Note that these steps will work. The failure only comes if you trigger a reconfiguration. Then it tries patching again, finds out that it cannot and it breaks the build. See screenshot above. We are assuming that you are running Linux or macOS. We recommend that Windows users rely on WSL. Follow the following two steps:
git clone https://github.com/simdjson/experimental_json_builder.git
bash run_docker.sh bash This will enter a bash shell with access to the repo directory. Note that this will take some time when running it for the first time, since the specific container image has to be built. This step builds and executes a docker container defined by our Dockerfile which provides the necessary environment.
cmake -B build This only needs to be done once.
cmake --build build
ctest --test-dir build --output-on-failure
./build/benchmarks/src/SerializationBenchmark You can modify the source code with your favorite editor and run again steps 6 (Build the code) and 7 (Run the tests) and 8 (Run the benchmark).
I do not. |
This. I do not know why; however, when I remove my Before we commit your change, we need to write up and understand this new issue. I won't have time to dig into this for a little while. :-( Maybe not even for a couple weeks if it's a complicated problem. :-( As a short term workaround you can set a cache directory as so: But here's my thoughts on how we should test this:
|
I'm going to try to work on this this weekend. |
I wonder if |
@lemire Maybe we should convert this to a draft? |
@ScottBailey I have updated the PR. If you use |
I'd like you to consider it. You can reject/close the PR, of course. |
No, actually I can't! I'm not a maintainer, just an interested party! :-) Patching is a problem within CMake itself, there could be a "baked in" tool that "just works". In fact, there will be one day, but not yet. When we get that, we will rework the PATCHES option. For now, PATCHES is a bit of a work around for a broken part of CMake proper. It gives us the functionality we expect from a package manager while we wait for someone to fix this upstream. (I've looked into it a bit, but I don't have the bandwidth to implement the change CMake's maintainers want. PATCHES is the compromise.) Regarding this PR, I have a few thoughts. I hope you'll consider them. The problem you've written this PR to address is not with the call to In package management, we might want to take a source (some_lib-v1.2.tgz) and apply patches (some_CVE.patch). I don't want a system that will fail to apply the patch without error and compile the library. What happens if the patch is for a critical vulnerability and it isn't applied? For this reason, if I were a maintainer here, I'd reject this PR. I started working #577, but it's gonna take me some time to figure it out. In the meantime, obviously, this is broken for you. Well, we have the one workaround: set But that might not be enough for you. Maybe you could add a |
Closing. |
Recent work by @ScottBailey making applying patches easy. However, I find that if the configuration is triggered more than once (necessary if the config changed), the tool may sometimes try to patch again the dependency. It then fails and the
cmake —build
command fails. It should be fine to just abort if the patch has already been applied, which is what--forward
flag should do. We use-N
as a shorthand for--forward
in the code.