feature: allow URI to use shorthand syntax with additional options #617

SGSSGene · 2024-11-07T10:06:41Z

(This is an alternative to PR #546 )
This allows to combine the shorthand syntax by using URI with additional arguments:

CPMAddPackage(URI "gh:nlohmann/[email protected]" OPTIONS "JSON_BUildTests OFF")

This is much shorter than the longer syntax way of writing:

CPMAddPackage (
  NAME nlohmann_json
  VERSION 3.9.1
  GITHUB_REPOSITORY nlohmann/json
  OPTIONS
    "JSON_BuildTests OFF"
)

~~Important difference to #546, it does not set EXCLUDE_FROM_ALL;YES;SYSTEM;YES;~~

TheLartians

Hey, thanks a lot for this alternative implementation and thanks for your patience, as I'm quite busy these days! I personally much prefer this alternative to the other PR, as it looks much cleaner and is not mixing different ways to pass arguments (I know it's a common CMake style, so this is a very subjective take). I was hoping to hear some input from @iboB on this, but I think he is very busy as well.

Important difference to #546, it does not set EXCLUDE_FROM_ALL;YES;SYSTEM;YES;

Can you elaborate what the advantage is? I think I it would be intuitive for both CPMAddPackage("gh:nlohmann/[email protected]") and CPMAddPackage(URI "gh:nlohmann/[email protected]") to work the same way, so IMO both versions should set the same flags.

Sorry again for taking so long to respond. I have some more time during the holidays, so happy to help get this merged asap.

test/integration/test_simple.rb

README.md

TheLartians · 2024-12-22T11:35:19Z

Can you elaborate what the advantage is?

I think I found the answer in the other PR:

Harder to document. Currently we have two cases: 1. normal usage, 2. shorthand syntax. With URI we kind of have to split the 'normal usage' into two cases. I don't see how this can be documented in any understandable way. (Assuming we don't delete the 2. shorthand syntax)

I see your concern, though I feel it's quite easy to understand in the docs:

All packages added using the shorthand syntax will be added using the EXCLUDE_FROM_ALL and SYSTEM flag.
...
Additionally, the shorthand syntax can be combined with the other options from above[ using the URI argument]:

SGSSGene · 2024-12-22T21:35:11Z

PR #546 basic idea was, take the "short hand" syntax and allow extended options.
This PR took the standard approach and added a URI which is a short from for repo, name and version.
That is why I didn't touch EXCLUDE_FROM_ALL and SYSTEM.
But from what I understand, you would prefer a URI which allows a short form of repo, name and version + also sets EXCLUDE_FROM_ALL and SYSTEM.

I will adjust this PR accordingly

This allows to combine the shorthand syntax with URI and additional arguments: ``` CPMAddPackage(URI "gh:nlohmann/[email protected]" OPTIONS "JSON_BUildTests OFF") ``` This is much shorter than the longer syntax way of writing: ``` CPMAddPackage( NAME nlohmann_json VERSION 3.9.1 GITHUB_REPOSITORY nlohmann/json OPTIONS "JSON_BuildTests OFF" ) ```

SGSSGene · 2024-12-23T11:01:53Z

This new version automatically sets EXCLUDE_FROM_ALL and SYSTEM. I made sure that this can be overridden by setting EXCLUDE_FROM_ALL OFF or SYSTEM OFF. (And added a unit/integration test for this case).

I added another note below the CPMAddPackage(....) description noting the behavior of URI.

TheLartians

Thanks for the updates! I really like how it's turning out. There are some small stylistic and documentation issues left and then I'm happy to merge this!

TheLartians · 2024-12-23T18:49:08Z

README.md

@@ -65,6 +65,7 @@ Afterwards, any targets defined in the dependency can be used directly.

 ```cmake
 CPMAddPackage(
+  URI           # shorthand including repo, name, version and tag (see shorthand syntax)


Thanks for adding the extra docs! Tbh I'm not sure if it won't be confusing that we show URI here, but also NAME and VERSION as only one or the other should be specified. Maybe we can leave it out here and keep this section as before to make it more obvious that the shorthand syntax is a way to replace the name and source info?

TheLartians · 2024-12-23T18:49:30Z

README.md

+CPMAddPackage(URI "gh:nlohmann/[email protected]"
+  OPTIONS "JSON_BuildTests OFF"
+)


Let's keep the formatting consistent with the other examples.

Suggested change

CPMAddPackage(URI "gh:nlohmann/[email protected]"

OPTIONS "JSON_BuildTests OFF"

)

CPMAddPackage(

URI "gh:nlohmann/[email protected]"

OPTIONS "JSON_BuildTests OFF"

)

TheLartians · 2024-12-23T18:52:35Z

README.md

@@ -112,6 +115,13 @@ CPMAddPackage("https://example.com/my-package-1.2.3.zip#MD5=68e20f674a48be38d60e
 CPMAddPackage("https://example.com/[email protected]")
 ```

+Additionally, the shorthand syntax can be used with the long version, using the `URI` specifier.


I think we can make it more clear that the new syntax allows for specifying additional arguments with the shorthand syntax:

Suggested change

Additionally, the shorthand syntax can be used with the long version, using the `URI` specifier.

Additionally, if needed, extra arguments can be provided by using the shorthand syntax with the `URI` specifier.

TheLartians · 2024-12-23T18:58:51Z

cmake/CPM.cmake

+  elseif(argnLength GREATER 1 AND "${ARGV0}" STREQUAL "URI")
+    list(REMOVE_AT ARGN 0 1) # remove "URI gh:<...>@version#tag"
+    cpm_parse_add_package_single_arg("${ARGV1}" ARGV0)
+
+    set(ARGN "${ARGV0};EXCLUDE_FROM_ALL;YES;SYSTEM;YES;${ARGN}")
+  endif()


We should either make it clear in the docs that URI can only be used as a first argument or alternatively, we can just add URI to the oneValueArgs and call cpm_parse_add_package_single_arg if CPM_ARGS_URI is defined.

TheLartians · 2024-12-23T18:59:57Z

test/integration/test_simple.rb

+      # we should end up with two executables
+      # * simple - the simple example from adder
+      # * using-adder - for this project
+      # ...and notably no test for adder, which must be disabled from the option override from above
+      assert_equal ['simple', 'test-adding', 'using-adder'], exes


The comment needs to be changed, as we are now expecting the tests to be built.

TheLartians · 2024-12-23T19:02:31Z

test/integration/test_simple.rb

+      exes = Dir[exe_dir + '/**/*'].filter {
+        # on multi-configuration generators (like Visual Studio) the executables will be in bin/<Config>
+        # also filter-out other artifacts like .pdb or .dsym
+        !File.directory?(_1) && File.stat(_1).executable?
+      }.map {
+        # remove .exe extension if any (there will be one on Windows)
+        File.basename(_1, '.exe')
+      }.sort


Not very important, but this code is now duplicated three times in the test case. Could it be more readable if we turn this it into a method and call it three times?

SGSSGene mentioned this pull request Nov 7, 2024

feature: allow additional arguments after shorthand syntax #546

Closed

SGSSGene force-pushed the feat/uri_with_options branch from 8686474 to 82b650b Compare November 7, 2024 10:11

TheLartians reviewed Dec 22, 2024

View reviewed changes

test/integration/test_simple.rb Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

SGSSGene added 5 commits December 23, 2024 11:09

fix: use shorthand syntax in examples

03c08f3

test: add test for shorthand syntax with options

0edaafc

doc: extend README mentioning shorthand syntax with options

72e98e7

feat: URI keyword also sets EXCLUDE_FROM AND SYSTEM

1cb8a5a

SGSSGene force-pushed the feat/uri_with_options branch from 82b650b to 1cb8a5a Compare December 23, 2024 10:50

doc: more explicit about the behavior of URI

32900e9

TheLartians requested changes Dec 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: allow URI to use shorthand syntax with additional options #617

feature: allow URI to use shorthand syntax with additional options #617

SGSSGene commented Nov 7, 2024 •

edited

Loading

TheLartians left a comment

TheLartians commented Dec 22, 2024

SGSSGene commented Dec 22, 2024

SGSSGene commented Dec 23, 2024

TheLartians left a comment

TheLartians Dec 23, 2024 •

edited

Loading

TheLartians Dec 23, 2024

TheLartians Dec 23, 2024

TheLartians Dec 23, 2024

TheLartians Dec 23, 2024

TheLartians Dec 23, 2024 •

edited

Loading

	Additionally, the shorthand syntax can be used with the long version, using the `URI` specifier.
	Additionally, if needed, extra arguments can be provided by using the shorthand syntax with the `URI` specifier.

feature: allow URI to use shorthand syntax with additional options #617

Are you sure you want to change the base?

feature: allow URI to use shorthand syntax with additional options #617

Conversation

SGSSGene commented Nov 7, 2024 • edited Loading

TheLartians left a comment

Choose a reason for hiding this comment

TheLartians commented Dec 22, 2024

SGSSGene commented Dec 22, 2024

SGSSGene commented Dec 23, 2024

TheLartians left a comment

Choose a reason for hiding this comment

TheLartians Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

TheLartians Dec 23, 2024

Choose a reason for hiding this comment

TheLartians Dec 23, 2024

Choose a reason for hiding this comment

TheLartians Dec 23, 2024

Choose a reason for hiding this comment

TheLartians Dec 23, 2024

Choose a reason for hiding this comment

TheLartians Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

SGSSGene commented Nov 7, 2024 •

edited

Loading

TheLartians Dec 23, 2024 •

edited

Loading

TheLartians Dec 23, 2024 •

edited

Loading