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

libmodulemd 2.x doesn't handle repodata where there are multiple stream documents for separate arches of the same NSVC #212

Closed
sgallagher opened this Issue Mar 6, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@sgallagher
Copy link
Collaborator

sgallagher commented Mar 6, 2019

This may require a major-version soname bump. Try to avoid this if at all possible.

@sgallagher sgallagher added this to the 2.2.0 milestone Mar 7, 2019

@sgallagher sgallagher added the Blocker label Mar 7, 2019

@sgallagher

This comment has been minimized.

Copy link
Collaborator Author

sgallagher commented Mar 7, 2019

The current behavior here is as follows: while processing through the YAML, we add each stream seen for a given module one at a time to a GPtrArray internally. In the current code, the modulemd_module_add_stream() function checks whether the NSVC has been seen before and if it is, it drops the previously-seen one and adds the new one. The correct behavior here would be for us to check for the same NSVCA and replace it if we've seen it before.(*)

So there are three functions that need changes:

  1. modulemd_module_add_stream() needs to not clobber streams with the same NSVC and different architecture.
  2. modulemd_module_get_stream_by_NSVC() needs to be deprecated in favor of modulemd_module_get_stream_by_NSVCA(). In order to not break any existing users of this function, I'll keep the current (sub-par) behavior of always returning the last matching entry in the array of streams. The modulemd_module_get_stream_by_NSVC() function will be annotated to warn that it is deprecated and the NSVCA variant should be used.
  3. modulemd_module_stream_get_nsvc_as_string needs to be deprecated in favor of modulemd_module_stream_get_nsvca_as_string(). Alternately, we could keep the current name, as its semantics already describe returning whichever subset of NSVC is available, so maybe it's okay to just have it return the NSVCA without introducing a new function. Suggestions welcome.

Seeking feedback from @Conan-Kudo @ignatenkobrain @contyk

(*) We could treat this as an invalid YAML stream, but the 2.0 API up to this point has just been treating it as "last one seen wins", so I don't think I want to change those semantics. I can be convinced otherwise. I'm only aware of two consumers of the 2.0 API currently (the fedora-module-defaults CI tests and a simple merging helper, neither of which uses the two deprecated functions), so if we decide to throw an error on duplicate NSVCAs, it might not be too much breakage to fix.

@Conan-Kudo

This comment has been minimized.

Copy link
Collaborator

Conan-Kudo commented Mar 7, 2019

so if we decide to throw an error on duplicate NSVCAs, it might not be too much breakage to fix.

I'd err on the side of caution and throw errors instead. Let's not allow "surprises" in the metadata generation. :)

@sgallagher

This comment has been minimized.

Copy link
Collaborator Author

sgallagher commented Mar 7, 2019

Yeah, I'm continuing to lean in that direction. The modulemd_module_add_stream() does have a GError return, so any consumer should be prepared to deal with errors here.

@Conan-Kudo

This comment has been minimized.

Copy link
Collaborator

Conan-Kudo commented Mar 8, 2019

With regards to modulemd_module_get_stream_by_NSVC and modulemd_module_stream_get_nsvc_as_string, we should just go ahead and deprecate them. I suspect if we made the latter function return NSVCA, bad things might happen due to the mismatch of expectations, and given the low usage of these functions, it's just better off to fix the problem with new functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.