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

Drop misuse of CMake's -H flag #9008

Closed
friendlyanon opened this issue Jun 14, 2022 · 5 comments
Closed

Drop misuse of CMake's -H flag #9008

friendlyanon opened this issue Jun 14, 2022 · 5 comments
Labels

Comments

@friendlyanon
Copy link

@friendlyanon friendlyanon commented Jun 14, 2022

Reference: #8982 (comment)

For justification, see this commit microsoft/vscode-cmake-tools@2c181de and the issue it closes.

A very quick search reveals 4 usages. They should be replaced according to what version of CMake is invoked at usage site. Details for how to proceed are also contained in the above referenced commit.

@bagder bagder added the cmake label Jun 14, 2022
@bagder
Copy link
Member

@bagder bagder commented Jun 14, 2022

My cmake manpage shows -H as an alias for --help, I guess that's not true 😬

@emanuele6
Copy link
Contributor

@emanuele6 emanuele6 commented Jun 14, 2022

that seems to actually be true when -H is used on its own; -Hdir is special 😆

@emanuele6
Copy link
Contributor

@emanuele6 emanuele6 commented Jun 14, 2022

The option that should be used instead of -H is -S, but all the uses of cmake -Hdir in curl are cmake -H. -Bsomething which, according to the man page, is equivalent to just cmake -Bsomething.

We can either remove -H. or replace it with -S..

Removing -H. instead of replacing it with -S. would be more portable since -S was only introduced in cmake v3.10.0 (released 2018-11-30).

curl's CMakeList.txt file currenly supports cmake v3.2.0 or newer, so the "remove -H." solution should be preferred.

cmake_minimum_required(VERSION 3.2...3.16 FATAL_ERROR)

Also, when -S/-Hdir are not used, the source directory can be specified by passing a path to cmake as a non-option argument; this approach, as far as I can tell, is probably a better to replace commands like cmake -H. -Bbuild $opts:

  • cmake -Bbuild $opts will only use the current working directory as the source directory if no non-option arguments were passed; if $opts happens to contain a non-option argument for some reason, the first non-option argument generated by the expansion of $opts will be used as source directory (if it exists).

  • cmake -Bbuild . $opts will instead make cmake use . as source directory and, if there are any more non-option arguments, print a warning message saying that the extra arguments were ignored.

emanuele6 added a commit to emanuele6/curl that referenced this issue Jun 14, 2022
This is an undocumented option, but behaves similarly to the `-S <path>`
introduced in cmake 3.10.

Since curl's current CMakeLists.txt file supports cmake 3.2 or newer,
we are replacing `-Hsdir -Bbdir` with the more portable `-Bbdir sdir`
instead of `-Sbdir -Bbdir`.

Fixes curl#9008
emanuele6 added a commit to emanuele6/curl that referenced this issue Jun 15, 2022
`-Hdir` is an undocumented option that behaves similarly to the
`-S <path>` option introduced in cmake 3.10.

Since curl's current CMakeLists.txt file supports cmake 3.2 or newer,
we are replacing `-Hsdir -Bbdir` with the more portable `-Bbdir sdir`
instead of `-Sbdir -Bbdir`.

Fixes curl#9008
@friendlyanon
Copy link
Author

@friendlyanon friendlyanon commented Jun 15, 2022

The origin of this functionality is Kitware/CMake@72bc795#diff-9451a015783be6eaba0a5a470186420c922d536fc465b80a4cdba55dec61d749R106, however as you noted this was never documented nor really supported.

cmake -H. -Bsomething which, according to the man page, is equivalent to just cmake -Bsomething.
...
since -S was only introduced in cmake v3.10.0 (released 2018-11-30).

-B and -S were added in CMake 3.13: https://cmake.org/cmake/help/latest/release/3.13.html#command-line

Before that, the current working directory was the binary directory and the passed path argument was the source directory.

curl's CMakeList.txt file currenly supports cmake v3.2.0 or newer

This is not a change that is concerned with the project code at all. This is purely about how cmake is invoked on the command line or in scripts.

@emanuele6
Copy link
Contributor

@emanuele6 emanuele6 commented Jun 15, 2022

This is not a change that is concerned with the project code at all. This is purely about how cmake is invoked on the command line or in scripts.

I am aware that is not a problem since these are CI script, that only run on machine with modern cmake versions. I just thought that writing a 3.2.0 command line would be more coherent since it seemed trivial to do.

cmake -H. -Bsomething which, according to the man page, is equivalent to just cmake -Bsomething.
...
since -S was only introduced in cmake v3.10.0 (released 2018-11-30).

-B and -S were added in CMake 3.13: https://cmake.org/cmake/help/latest/release/3.13.html#command-line

You are right, -B and -S were added together in CMake 3.13 (not 3.10), so it doesn't really make sense to use one, but not the other.

emanuele6 added a commit to emanuele6/curl that referenced this issue Jun 15, 2022
This is an undocumented option similar to the `-Spath` option intruced
in cmake 3.13.
Replace all instances of `-Hpath' with `-Spath'.

Fixes curl#9008
emanuele6 added a commit to emanuele6/curl that referenced this issue Jun 20, 2022
This is an undocumented option similar to the `-Spath' option intruced
in cmake 3.13.
Replace all instances of `-Hpath' with `-Spath' in macos workflow.
Replace `-H. -Bpath' with `mkdir path; cd ./path; cmake ..' in zuul
scripts since it runs an older version of cmake.

Fixes curl#9008
emanuele6 added a commit to emanuele6/curl that referenced this issue Jun 21, 2022
This is an undocumented option similar to the `-Spath' option introduced
in cmake 3.13.
Replace all instances of `-Hpath' with `-Spath' in macos workflow.
Replace `-H. -Bpath' with `mkdir path; cd ./path; cmake ..' in zuul
scripts since it runs an older version of cmake.

Fixes curl#9008
@bagder bagder closed this as completed in 79f915e Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants