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

Allow defining custom prefixes #445

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

papperlapapp
Copy link

In addition to the predefined prefixes 'gh', 'gl' and 'bb', allow defining your own prefixes for, e.g. internal hosts.

@TheLartians
Copy link
Member

TheLartians commented Feb 20, 2023

Hey thanks for the PR! Changes look good to me though I'd like an additional opinion from @iboB as he authored the original feature. Could you also format the code using according to the styleguides?

In addition to the predefined prefixes 'gh', 'gl' and 'bb', allow
defining your own prefixes for, e.g. internal hosts.
Copy link
Member

@iboB iboB left a comment

Choose a reason for hiding this comment

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

I have several problem with this as it seems kinda incomplete. I'm not sure whether we want to pursue a more "complete" feature with this.

  1. I'm not sure I like the name "prefix" in this case. Maybe schemes?
  2. As I said, I don't like the addition of CUSTOM_REPOSITORY. The entire uri is contained in the long-form anyway. Adding CUSTOM_REPOSITORY to split in in parts seems redundant and unneeded.
  3. CPM_CUSTOM_PREFIXES seems to imply git. This is easily fixable as we have tools to detect the type of the repo based on the uri. Alternatively it should be CPM_CUSTOM_GIT_REPO_PREFIXES
  4. These are prefixes only. So if I wanted to add my custom GitLab instance I would have to manually add .git at the end of the uris, like:
CPMAddPackage(my:foo/bar.git@1.2.3) # :(
CPMAddPackage(gh:foo/bar@1.2.3) # :)

I can't think of a good way to fix this. Something like CPM_CUSTOM_SUFFIXES perhaps?...

foreach(prefix ${CPM_CUSTOM_PREFIXES})
if("${prefix}" MATCHES "([^:]+):(.+)")
if(scheme STREQUAL "${CMAKE_MATCH_1}")
set(out "GIT_REPOSITORY;${uri};CUSTOM_REPOSITORY;${CMAKE_MATCH_2}")
Copy link
Member

Choose a reason for hiding this comment

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

why introduce CUSTOM_REPOSITORY at all? It seems somewhat redundant to me

Why not concat the string here: "GIT_REPOSITORY;${CMAKE_MATCH_2}/${uri}"?

Copy link
Author

Choose a reason for hiding this comment

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

Because I didn't knew it better. This was the first time I fiddled with CMake functions.

@iboB
Copy link
Member

iboB commented Feb 21, 2023

Here's how I would do it:

CPMAddCustomScheme(
    c1 # scheme name
    PREFIX https://c1.example.com/ # optional
    SUFFIX .git # optional
    TYPE GIT_REPOSITORY # optional. if not specified, tries to infer the type from the composed uri
)

# ...

CPMAddPackage(c1:foo/bar@1.2.3) # :)

If this is implemented, the existing default schemes can be added to the same underlying lists that this is using.

It can even allow overriding of existing schemes and some of the functionality requested in #333

@papperlapapp
Copy link
Author

papperlapapp commented Feb 21, 2023

This looks definitely better than my attempt. I already had the feeling that a generic way of implementing this should somehow be possible.

Time for diving deeper into the CMake documentation ...

Shall I close the PR and start anew, or just keep it open and work on it (will definitely take its time)?

@iboB
Copy link
Member

iboB commented Feb 21, 2023

Awesome! It's great that you want to invest more in this

Shall I close the PR and start anew, or just keep it open and work on it (will definitely take its time)?

I have no special opinion on the matter. However you prefer

@CraigHutchinson
Copy link
Contributor

CraigHutchinson commented Mar 21, 2023

I think this change has really useful potential, something I had considered as would simplify setup with enterprise servers.

Ideally all the existing prefix/schema types should be migrated to use this mechanism imho. Partly as a basis of solid testing to harden the design but also as it may be desirable to modify existing behavior on the pre-existing schemas!

For example, I was considering allowing to provision alternative server addresses or fallbacks when the primary are not accessible e.g.CPM_GITHUB_SERVER=local-github.internal;central-github.internal;github.com so CPM will check in order precedence etc. There are definitely edge cases to consider here but if the prefix/schema had a 'function' aspect akin to dependency_provider then this may be super power making the core of CPM technically more modular in this area.

EDIT: I should note I haven't thought much on the example as I think it falls more into ExternalProject functionality to have alternatives for GIT as it does for URL downloads:

URL <url1> [<url2>...]
List of paths and/or URL(s) of the external project's source. When more than one URL is given, they are tried in turn until one succeeds.

@ClausKlein
Copy link
Contributor

Would this MR #528 be a the simpler solution for your problem?

@k-channon-PA
Copy link

I just implemented a similar thing to this... and then checked the open PRs only to find this one!

a6cc6b9

This feature would really help me out. I'm happy to help get this PR completed if that'a any use to you guys :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants