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

ambiguous on disk names for component and configurations vs packages with - in their name #79

Open
dcbaker opened this issue Oct 7, 2024 · 5 comments

Comments

@dcbaker
Copy link
Collaborator

dcbaker commented Oct 7, 2024

Looking at the spec for configuration merging it specifies that once a package <name>.cps is found, then the following packages in the sibling directory should be loaded and merged:

  • <name>:*.cps (except on Windows)
  • <name>-*.cps
  • <name>@*.cps
  • <name>:*@*.cps (except on Windows)
  • <name>-*@*.cps

Per the documentation of package names (https://cps-org.github.io/cps/components.html), it appears that @ and : are invalid in the package name, so should be invalid in the on-disk name as well? So a package called code::blocks would be invalid, it would need to be called something else, like code_blocks. So that seems like maybe a chance to clarify the spec?

The bigger concern I have is that - is allowed in package names, which means for a cps file installed directly in a cps prefix root, like /usr/lib/cps/ there is a potential for ambiguous names to cause collisions, where a - is used to name a third party component, rather than a configuration or component fragment. (I will refer to these component and configuration .cps files as fragments)

Imagine:

/usr/lib/cps/zlib.cps
/usr/lib/cps/zlib-ng.cps

these are two separate projects, with two separate implementations, and should not be combined, but it's impossible to tell that without opening and reading both of them to determine that zlib-ng is not a fragment of zlib

I really don't like the idea that we have to fully parse zlib-ng.cps in order to reject it, it makes for longer search times and more disk i/o. A few possible solutions:

  1. require that all .cps files be installed in a subfolder such that zlib/*.cps and zlib-ng/*.cps cannot be confused
  2. drop - as a fragment separator, such that only : and @ are valid. This means that we need a replacement for windows, since : isn't valid?
  3. give fragments a different extension, like .cpf
  4. Something else?
@dcbaker
Copy link
Collaborator Author

dcbaker commented Oct 8, 2024

The more I think about it the more I like option 3, since it has the added benefit of simplifying searching for "core" cps files as well as simplifying searching for fragments.

@mwoehlke
Copy link
Member

a package called code::blocks would be invalid, it would need to be called something else, like code_blocks

Correct. More to the point, your package name is your namespace name, and :: isn't permitted in a namespace name; code::blocks* (mind the wildcard) is not a valid component name (in CPS), and code:blocks is the blocks component of the code package. (CMake will auto-translate code::blocks appearing in your CMakeLists.txt to CPS code:blocks.)

that seems like maybe a chance to clarify the spec?

I'm not sure what is being clarified. Are you looking for something like, "For example, while you may think of your package as my:cat, this is not a valid CPS package name" in Component Specification?

(On second though, I am trying to remember the rationale for allowing : in component names... Especially as that isn't compatible with CMake...)

I really don't like the idea that we have to fully parse zlib-ng.cps in order to reject it

...But how often is this likely to happen in practice? You can already keep your .cps files in a directory that contains your package name (like Option 1, except it isn't mandatory), and indeed I would encourage doing so, this being one (but not the only) reason. You also won't ever look at a zlib-ng.cps except when you've already decided to use the zlib.cps in the same directory, so I suspect the added overhead is not nearly so dire.

@dcbaker
Copy link
Collaborator Author

dcbaker commented Oct 22, 2024

I'm not sure what is being clarified. Are you looking for something like, "For example, while you may think of your package as my:cat, this is not a valid CPS package name" in Component Specification?

The only place I could find that was in the component name section, it would seem like it could be explained or cross-referenced under the searching section, or more prominently in its own section?

You also won't ever look at a zlib-ng.cps except when you've already decided to use the zlib.cps in the same directory, so I suspect the added overhead is not nearly so dire.

You'll load it as a fragment, and then have to have code hanlding to reject it as not a fragment. zlib-ng would be particularly problematic in this case, because they need to provide both a zlib.cps and a zlib-ng.cps, presumably side-by side, since the zlib.cps is for their zlib API compatible interface and zlib-ng.cps would be for their non-compatible API. I'm not even sure it would be reasonable for them to ship with a single .cps file at all, since you can't arbitrarily switch between the two components, you absolutely need one or the other.

If you ship multiple fragments of zlib-ng, there could be a worse confusion, merging a zlib-ng-*.cps into zlib.cps, and then using the wrong thing, imagine a situation like:

zlib.cps  # is release only
zlib-ng.cps
zlib-ng-debug.cps

and the user asks for zlib in a debug configuration, zlib.cps and zlib-ng-debug.cps both get loaded, and there is a debug configuration, so that's selected. Unfortunately, the user expected the classic zlib API from zlib.cps, and gets an zlib-ng library, and linking (or possibly compiling) fails. We could do a bunch of loading validation to ensure that everything is correct, but that's placing additional chances of failure on every implementation, and we could be solving that now by everyone by clarifying the rules a little bit in some way to ensure that the situation is invalid.

@mwoehlke
Copy link
Member

You'll load it as a fragment, and then have to have code handling to reject it as not a fragment.

Yes, but that code is a) trivial (exactly one check), and b) a sanity check that should be present regardless. It's also the same early sanity check that should be applied to the "root" CPS file (in this example, zlib.cps).

zlib.cps and zlib-ng-debug.cps both get loaded

Uh... no? Your implication is that zlib-ng-debug.cps doesn't belong to the zlib package, therefore the tool won't load anything from it.

(I'll ignore that you probably meant [email protected] as that doesn't seem to make a difference.)

they need to provide both a zlib.cps and a zlib-ng.cps, presumably side-by side

Why not follow Option 1?

@dcbaker
Copy link
Collaborator Author

dcbaker commented Oct 25, 2024

I'm sorry, I wrote up a reply yesterday, and apparently never posted it!

I thought about Option 1, and don't think it actually helps, consider the following:

cps/zlib/zlib.cps  # upstream, vanlila zlib
cps/zlib/zlib-ng.cps  # zlib-ng's zlib API
cps/zlib/zlib-component-1.cps  # zlib vanilla component
cps/zlib/zlib-ng-component-1.cps

vs

cps/zlib/zlib.cps
cps/zlib/zlib-ng.cps
cps/zlib/zlib:component-1.cps
cps/zlib/zlib-ng:component-1.cps

There's zero ambiguity here. We know right away that zlib: are zlib.cps components, and zlib-ng: are zlib-ng.cps components. We already have this for configurations, since @ is reserved for configurations. I don't foresee Meson generating any cps files with :, specifically because those files wouldn't be usable in cross compilation setups. But I really like the fact that @ and : are not ambiguous.

I see two problems with the current approach:

  1. we have to brute force all possible components, something we do not have to do with configurations
  2. we're pushing more code (and therefore more chances of incompatible behavior) onto to every implementation.

So what I'd propose is:

  1. drop : entirely. I propose this regardless of whether we accept 2
  2. replace - as a component separator with something that is non-ambiguous and is valid on all platforms. looking at candidates, $ seems like it might work?
  3. profit?

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

No branches or pull requests

2 participants