version feature naming restriction is problematic

[jnqnfe]

Hi,

I'm exploring switching my libpulse_sys crate and its companions over to system-deps from pkg-config. Converting to v1.3 it seems to work, but upgrading to v2.0 I'm getting an error:

cargo:warning=Unexpected key package.metadata.system-deps.libpulse.pa_v12 type table

Could you please take a look.

I've pushed the change to my next branch: https://github.com/jnqnfe/pulse-binding-rust/tree/next

The second to last commit there is the conversion to v1.3 and the last commit is the problematic upgrade to v2.0.

Thanks. :)

[jnqnfe]

Looking into this a little more, digging through the commits that went into v2, it seems that the feature-versions design of v1 supported feature flags of any naming scheme, while the redesign in v2 does not, now only being compatible with crates using a scheme starting with a 'v', which is the reason behind the error (it confused me before since v1 supported my flags, nothing really said that flags of a difference scheme would not work with v2, and the error wasn't referencing the first feature+version entry I specified but one in the middle of the set, which I guess is due to alphabetic sorting).

My crates do not conform to the 'v1_0' naming scheme, being of the form 'pa_v1'. This makes v2 impossible to use for my sys crates without a whole mess of creating a duplicate set of flags starting with a 'v' which I'm unwilling to do.

Although the v2 documentation for the crate says that crates "should" use the 'v1_0' scheme, it does not state that you absolutely must, or rather does not specifically state that whatever scheme you use, that it must start with a 'v'. Nor does it mention there being any difference from past versions. This was unhelpful. Additionally it would have been helpful/nice if there were a changelog or migration-guide highlighting such compatibility breaking differences between the versions, to assist users of older versions know what they need to know to upgrade.

Now that I understand what the problem is, and considering that I can't just stick to v1 forever, and have no wish to switch naming schemes for my crate even if it were easy to do, I'll have a look at your code and have a think about how to remove the restriction you introduced under the new design.

Going a little off topic now, but well...

I'm also interested in how exactly version checks are being handled with respect to feature combinations. From your versioning example in the documentation, flag v1_6 enables v1_4 which enables v1_2, which is the same sort of pattern I use. But does use of flag v1_6 thus mean that your crate is then having to make three different version queries through pkg-config? I assume, without having assessed the code yet that it must, which is not ideal. Perhaps the only good solution to this would be to use something like .cfg(not(v1_6)) in the section for v1_4 and similarly .cfg(not(v1_4)) for v1_2, if that works. If that does work, then perhaps that's work talking about in the documentation. Though of course making multiple queries here is not a huge deal.

Edit: I should have read the docs more carefully before writing that paragraph, they clearly state that the highest version is picked.

Additionally, I'm not a fan of the replacement of feature-versions being to specify a whole bunch of new sections in toml files, one per feature flag, which is much messier than the v1 solution. I could do with refreshing my understanding of toml file formats, but is it not possible for entries under sections to use names that use fullstops to create sub-sections? A brief test seems to suggest that you can. If so, this means that a description like this is possible which is much, much neater:

[package.metadata.system-deps.gstreamer_1_0]
name = "gstreamer-1.0"
version = "1.0"
v1_2.version = "1.2"
v1_4.version = "1.4"
v1_6.version = "1.6"

Can that be combined with the .cfg(not(v1_6)) thing though? Can you do v1_2.version = "1.2" as well as v1_2.name = "foo"? I'll have to experiment...

If I'm correct, perhaps the documentation could be modified to cover this style instead or as well. It may help avoid potential users being put off using your crate by the messier form of version expression of v2.

Edit: I guess maybe this might be another way, or more correct way, of expressing the above, and addresses one of the two aspects I needed to check:

[package.metadata.system-deps.gstreamer_1_0]
name = "gstreamer-1.0"
version = "1.0"
v1_2 = { version = "1.2" }
v1_4 = { version = "1.4", name = "foo" }
v1_6 = { version = "1.6" }

[gdesmott]

Sorry about the bumpy transition to v2. I (wrongly) assumed that gtk-rs & friends were the only users of feature-versions and so that porting the toml generator to the new syntax was enough.

I'm enforcing the v* pattern because I did not want to blindy assume that any other key in the toml would always be a version identifier, as we may want to add more specific keys in the future. I made this clearer in the doc.
I'm open to discuss alternative and better way to handle that.

I agree that the latest example is simpler and more elegant so I updated the doc to use this pattern. Thanks for the suggestion.

system-deps has been mostly designed to fit the gtk-rs and gstreamer-rs use cases. I'm happy to consider improvement to fit your needs.

[jnqnfe]

Sorry about the bumpy transition to v2. I (wrongly) assumed that gtk-rs & friends were the only users of feature-versions and so that porting the toml generator to the new syntax was enough.

Well, no, you weren't wrong AFAIK; as I said, I was just the other day experimenting with switching to using system-deps from pkg-config, I wasn't already using it.

Edit: I hope that my earlier criticisms did not come across as overly harsh.

I'm enforcing the v* pattern because I did not want to blindy assume that any other key in the toml would always be a version identifier, as we may want to add more specific keys in the future. I made this clearer in the doc.
I'm open to discuss alternative and better way to handle that.

Ok, great :) I had intended to spend some time thinking it through on Monday, but I became swamped with stuff I'm still working through and so all I accomplished was familiarising myself with some of the code a little and producing a trivial formatting improvement which I've not even PR'd yet. I'll get back to this as soon as I can.

I agree that the latest example is simpler and more elegant so I updated the doc to use this pattern. Thanks for the suggestion.

👍

system-deps has been mostly designed to fit the gtk-rs and gstreamer-rs use cases. I'm happy to consider improvement to fit your needs.

Great 👍

[gdesmott]

@jnqnfe : any specific improvement you'd need to be able to use sytem-deps with libpulse_sys or should I close this ticket?

[jnqnfe]

@jnqnfe : any specific improvement you'd need to be able to use sytem-deps with libpulse_sys or should I close this ticket?

Hi, very sorry, my attention got taken completely away from this for much longer than I expected. I'm afraid that not expecting to take so long to respond, I did not bother to write down the possible concepts that I had thought of to address the problem, and my recollection of them has become a little hazy :/, but let's see...

So the key requirement was removing the feature naming restriction introduced in v2, but keeping in mind your forwards compatibility concern over the possibility of a need to introduce new keys in future.

In no particular order...

Solution 1

One thought was that version feature names usually if not always contain at least one numeric symbol, while your keys do not and highly likely never will. So a naming convention requirement of containing at least one numeric symbol instead of 'starts with a v' could perhaps be much better as a means of distinction in terms of flexibility. This would significantly relax current restrictions, allowing a much wider set of feature flag naming conventions to be compatible, such as mine, but whilst still enforcing a clear separation between feature flag naming and 'known key' naming to still address your forwards compatibility concern.

In fact your current solution to this forwards compatibility concern obviously only allows you to freely add new keys that do not start with a v, which is not exactly ideal. This alternative of being free to add keys that do not include numbers seems much less problematic to me.

Solution 2

Another possible idea might have been to provide a key with which to override the prefix checked for (v), but this might have been somewhat problematic and is a less than ideal solution.

Solution 3

Another idea was that it might simply be best to revert back to something more like what was done with v1:

[package.metadata.system-deps.gstreamer_1_0]
name = "gstreamer-1.0"
version = "1.0"
feature-versions = {
    v1_2 = { version = "1.2" }
    v1_4 = { version = "1.4", name = "foo" }
    v1_6 = { version = "1.6" }
}

Thus all keys under feature-versions can be safely treated as always being feature names, and clarity is improved.

I presume cargo toml allows this multi-line nested form?

Solution 4

A final solution was a change in logic based upon presuming that it is possible to check a string against the list of available feature names (is this possible?) in order to try to determine what each key is. So for each key:

• If it does not match a feature name or any known key (like 'name' or 'version'), then report an error.
• If it matches only a known key, then use it as such.
• If it matches only a feature name, then proceed on the basis of it being feature-version data.
• If it matches both, then either just error, or possibly try a little harder - consider whether it has a single value or a table, whether a table with a version subkey specifically perhaps.

Evaluation

• Solution 2 should probably just be disregarded.
• Solution 4 may have some promise, if say feature-versions always have tables and your keys do not, but it's not exactly perfect and this could break down in future. Besides, the essentially guaranteed use of numeric symbols in feature version flags means clashes are extremely unlikely to occur anyway, and so solutions 1 or 3 might be better.
• Solution 1 I feel is a good one, so long as it is okay to reject naming conventions that do not include numbers and to expect your keys to never include them. Doing a contains-number check obviously requires more work than 'starts with a v', but is worth the trade off, though we can avoid this work entirely with solution 3.
• Solution 3 is perhaps the best, with it's pure simplicity; better clarity; lack of any and all naming convention restrictions; avoidance of 'contains' checks and such; and complete avoidance of the forwards compatibility naming issue. The extra wrapping key in the example above is perfectly fine with me, I just lack certainty whether it is a permitted form; I would not like to go back to the two-section-heading form of v1.

So essentially I guess I'd prefer solution 3 if possible.

An aside

A thought - v1 simply used key=value style for each feature, whilst v2 switched to tables. The dependency keys in cargo toml files are flexible, allowing either just a version to be specified in key=value form, or a table to be provided where multiple key=value properties need to be given. It might make things cleaner if each feature-version key could similarly offer such flexibility.

[gdesmott]

We dropped feature-versions because more feature specific keys were being added and it was easier and more convenient to write them such as:

[package.metadata.system-deps.gstreamer_gl_wayland_1_0.v1_16]
version = "1.16"

[package.metadata.system-deps.gstreamer_gl_wayland_1_0.v1_18]
name = "gstreamer-gl-wayland-1.0"
version = "1.18"

So I'd rather not revert to the old format, discarding Solution 3.

I don't think we can (easily) get the list of all features and I'd rather not reparse the whole Cargo.toml to gather it manually. So solution 4 seems tricky to implement.

That leaves us with solution 1, but we are replacing one syntax trick but another one. It will fix your specific use case but not the overall issue.

@sdroege : any smart idea here? :)

[gdesmott]

We have to be careful with assuming that keys containing numeric symbols are versions. With #33 we can now have this for example:

[package.metadata.system-deps.'cfg(target_pointer_width = "64")']