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.

Sign up for GitHub

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

Jump to bottom

cmake: Move runtime output artifacts to ${PROJECT_BINARY_DIR} #1233

Closed
wants to merge 2 commits into from
Closed

cmake: Move runtime output artifacts to ${PROJECT_BINARY_DIR} #1233

wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 10, 2023 •
edited
Loading

From IRC:

12:32 < sipa> With cmake, the build artefacts seem to end up in build/src/, while I'd expect them in build/.

Implemented in this PR.

On Windows, this also simplifies running examples, because the DLL must resides either in the same folder where the executable is or somewhere in PATH.

real-or-random
real-or-random reviewed Mar 10, 2023
View reviewed changes
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Hm, I don't know. This seems a bit arbitrary.

If we now deviate from putting the artifacts in their respective source directory anyway, and setting the artifact path is just setting a variable, would it make sense to also just put anything in ${PROJECT_BINARY_DIR} (or in a dedicated subdir of that)?

@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2023 •
edited
Loading

Hm, I don't know. This seems a bit arbitrary.

Agree. But adding each path for every configuration to PATH, which is the case for multi-config, is really annoying.

If we now deviate from putting the artifacts in their respective source directory anyway, and setting the artifact path is just setting a variable, would it make sense to also just put anything in ${PROJECT_BINARY_DIR} (or in a dedicated subdir of that)?

Like ${PROJECT_BINARY_DIR}/out? Or ${PROJECT_BINARY_DIR}/bin?

@real-or-random
Copy link
Contributor

Agree. But adding each path for every configuration to PATH, which is the case for multi-config, is really annoying.

Indeed. It's also annoying for devs on Windows (though noone uses Windows right now...)

Like ${PROJECT_BINARY_DIR}/out? Or ${PROJECT_BINARY_DIR}/bin?

Yeah, I had something like this in mind. out is probably a bit nicer because bin looks wrong for .so etc. Just not convinced it's worth it. Having a subdir is a cheap and feels tiny bit cleaner because it avoids name collisions with existing files (e.g., what if we have a binary examples). Though in that case, autotools would break anyway... I guess there's no clear right or wrong here, but if we want to move all artifacts to a single location, then ${PROJECT_BINARY_DIR} is probably good enough, and then it matches the autotools behavior at least. As discussed in IRC, we don't care much and should do whatever makes things easier.

hebasto added 2 commits March 10, 2023 17:42
@hebasto
cmake: Move runtime output artifacts to ${PROJECT_BINARY_DIR}
46ecb36 
This change (1) simplifies running examples on Windows, because the DLL
must resides either in the same folder where the executable is or
somewhere in PATH; and (2) mimics Autotools-based build system behavior.
@hebasto
ci: Drop no longer needed code
9dae692
@hebasto hebasto changed the title cmake: Move example binaries to ${PROJECT_BINARY_DIR}/src cmake: Move runtime output artifacts to ${PROJECT_BINARY_DIR} Mar 10, 2023
@hebasto
Copy link
Member Author

hebasto commented Mar 10, 2023

... if we want to move all artifacts to a single location, then ${PROJECT_BINARY_DIR} is probably good enough, and then it matches the autotools behavior at least.

Done.

The PR description has been updated (friendly ping @sipa :D ).

@hebasto hebasto marked this pull request as draft April 12, 2023 11:16
@hebasto
Copy link
Member Author

hebasto commented Apr 12, 2023

Converted to draft for now.

@theuni
Copy link
Contributor

theuni commented Apr 27, 2023

Rather than setting the PATH in CI, can we set it in CMake for make check?

@real-or-random
Copy link
Contributor

Rather than setting the PATH in CI, can we set it in CMake for make check?

I think this makes sense, and then Concept NACK on moving the binaries

@hebasto hebasto mentioned this pull request Apr 29, 2023
Merged
@hebasto
Copy link
Member Author

hebasto commented Apr 29, 2023

@theuni @real-or-random

Rather than setting the PATH in CI, can we set it in CMake for make check?

Implemented in #1290.

Closing in favour of #1290.

@hebasto hebasto closed this Apr 29, 2023
@hebasto hebasto deleted the 230310-output branch April 29, 2023 09:30
real-or-random added a commit that referenced this pull request Aug 3, 2023
@real-or-random
Merge #1290: cmake: Set ENVIRONMENT property for examples on Windows
374e2b5 
175db31 ci: Drop no longer needed `PATH` variable update on Windows (Hennadii Stepanov)
116d2ab cmake: Set `ENVIRONMENT` property for examples on Windows (Hennadii Stepanov)
cef3739 cmake, refactor: Use helper function instead of interface library (Hennadii Stepanov)

Pull request description:

  This PR simplifies running examples on Windows, because the DLL must reside either in the same folder where the executable is or somewhere in PATH.

  It is an alternative to #1233.

ACKs for top commit:
  real-or-random:
    utACK 175db31

Tree-SHA512: 8188018589a5bcf0179647a039cdafcce661dc103a70a5bb9e6b6f680b899332ba30b1e9ef5dad2a8c22c315d7794747e49d8cf2e391eebea21e3d8505ee334b
@hebasto hebasto mentioned this pull request Jun 26, 2024
Merged
real-or-random added a commit that referenced this pull request Oct 15, 2024
@real-or-random
Merge #1553: cmake: Set top-level target output locations
e59158b 
c232486 Revert "cmake: Set `ENVIRONMENT` property for examples on Windows" (Hennadii Stepanov)
26e4a7c cmake: Set top-level target output locations (Hennadii Stepanov)

Pull request description:

  While testing #1551, I noticed that when cross-compiling a shared library with examples for Windows, the `ctest` fails to run examples with Wine. Adjusting the `PATH` variable in https://github.com/bitcoin-core/secp256k1/blob/4af241b32099067464e015fa66daac5096206dea/examples/CMakeLists.txt#L16-L18 does not help because `WINEPATH` is expected.

  Another issue with the current implementation is that the examples cannot run individually on Windows.

  This PR resolves both issues by reverting the implementation from #1290 in favour of the reworked and improved implementation from #1233.

ACKs for top commit:
  theuni:
    Concept ACK and utACK c232486.
  real-or-random:
    utACK c232486

Tree-SHA512: 479b71d15d5d5670f6f69da3da599240c345711003383ca805c821b67065c9baaf269f987792cf1029211cdbfe799aecd401e6940a471539e3929b4a90e0781d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@real-or-random real-or-random real-or-random left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hebasto @real-or-random @theuni