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

modules: Fix test command when library output path is set #77

Closed

Conversation

@Squareys
Copy link
Contributor

Squareys commented Sep 15, 2019

Hi @mosra !

This is a tiny one, but would help me with maintenance a bit if it were to be upstreamed :)

Best,
Jonathan

From the commit message:

E.g. when `LIBRARY_OUTPUT_PATH` is set globally or the target property
`LIBRARY_OUTPUT_DIRECTORY` is set.

add_test(<name> <command> <args>) does not resolve the target file, but
rather just runs the command, which works as long as it is where cmake
puts in by default, but not if the library output dir is modified.

add_test(NAME <name> COMMAND <command> <args>) on the other hand resolves
generator expressions and <command>, if an executable target, will
"automatically be replaced by the location of the executable created at
build time." (from the cmake docs.)
@Squareys Squareys force-pushed the Squareys:use-corrade-with-library-path branch from 77102ed to 8cc50fe Sep 15, 2019
@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 15, 2019

Unlike the above NAME signature no transformation is performed on the command-line to support target names or generator expressions.

cmake what the hell.

Can you add a changelog entry as well so I don't have to? :)

@mosra mosra added this to the 2019.0b milestone Sep 15, 2019
@mosra mosra added this to TODO in Project management via automation Sep 15, 2019
@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Sep 16, 2019

Sure!

cmake what the hell.

That's documented, though :D

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 16, 2019

I mean, I see no reason at all why the shorthand version would need to behave so differently.

E.g. when `LIBRARY_OUTPUT_PATH` is set globally or the target property
`LIBRARY_OUTPUT_DIRECTORY` is set.

add_test(<name> <command> <args>) does not resolve the target file, but
rather just runs the command, which works as long as it is where cmake
puts in by default, but not if the library output dir is modified.

add_test(NAME <name> COMMAND <command> <args>) on the other hand resolves
generator expressions and <command>, if an executable target, will
"automatically be replaced by the location of the executable created at
build time." (from the cmake docs.)

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys Squareys force-pushed the Squareys:use-corrade-with-library-path branch from 8cc50fe to 114e58d Sep 16, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 16, 2019

Codecov Report

Merging #77 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #77   +/-   ##
=======================================
  Coverage   97.76%   97.76%           
=======================================
  Files          86       86           
  Lines        5732     5732           
=======================================
  Hits         5604     5604           
  Misses        128      128

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c577cd...114e58d. Read the comment docs.

@Squareys

This comment has been minimized.

Copy link
Contributor Author

Squareys commented Sep 16, 2019

Done ✔️

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Sep 20, 2019

Merged as 9298434, thanks!

@mosra mosra closed this Sep 20, 2019
Project management automation moved this from TODO to Done Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.