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

Proposal: handling conflicting libraries in the same package #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

scheibelp
Copy link
Member

@scheibelp scheibelp commented May 11, 2021

Propose a change to the variant declaration syntax to support packages which supply multiple instances of one library, each built differently. For more details, read the proposal.

See also: spack/spack#22749

Comment on lines +38 to +39
variant('threading', values=['openmp', 'sequential'],
interface=True)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have a more generic argument that can take a single value or a list of them:

Suggested change
variant('threading', values=['openmp', 'sequential'],
interface=True)
variant('threading', values=['openmp', 'sequential'], type='interface')

In this way we would use a single directive to declare variants that are relevant for both deploying a package and using it. With the proposed interface such a case would need two declarations:

variant('libs', values=('shared', 'static'), multi=True, interface=True)
variant('libs', values=('shared', 'static'), multi=True, interface=False)

while using a more generic argument one suffices:

variant('libs', values=('shared', 'static'), multi=True, type=('deploy', 'interface'))

Another minor benefit of this would be that we'll leave the type of variants open for extensions, even though it's very likely that with these two different types ("deploy" and "interface") we'll be good for a long time.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this though not necessarily on the names "interface" and "deploy", but we can work those out later.

Copy link
Member

Choose a reason for hiding this comment

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

@tgamblin Sure, even "type" as the name of the argument is just a placeholder at the moment.

seps/sep-0002.md Outdated
Comment on lines 25 to 28
* During concretization, this variant can be treated like any other
variant.
* After concretization, the variant is stripped from the node and
stored on the edge connecting the node to its parent.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the optimal way of proceeding with the original concretizer. It should work also with the ASP-based solver, but I think in that case it would be better to model the semantics in the logic program (i.e. in concretize.lp). @tgamblin Do you have already some thoughts on it?

Copy link
Member

Choose a reason for hiding this comment

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

I think this doesn't handle all the cases yet -- in particular, we discussed wanting to be able to constrain how some use variants were used but not others. If we keep the use-variant on the spec, and only strip it off after, then we're forcing all of them to be unified. But I think you could have cases where one dependent uses, e.g., a boost library and another does not -- and that could be perfectly fine. This spec doesn't yet seem to cover how to constrain use variants .. maybe @becker33 could take a stab at that as it was one of his examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also discuss Boost as a use case and came to the conclusion that it is resolvable in a simpler manner (which I describe there). I intended to propose that use variants (for other readers, that is variants with interface=True) should be treated exactly like variants during concretization.

@alalazo
Copy link
Member

alalazo commented May 12, 2021

By the way, thanks @tgamblin for starting this repo and thanks @scheibelp for submitting this PR. My feeling is that this will help a lot to design new major features.

@tgamblin tgamblin self-assigned this May 19, 2021
@scheibelp

This comment has been minimized.

* During concretization, this variant can be treated like any other
variant.
* After concretization, the variant is stripped from the node and
stored on the edge connecting the node to its parent.
Copy link
Member

Choose a reason for hiding this comment

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

I think there are three features mentioned in this SEP, but their semantics aren't really laid out here. I see:

  • interface=True
  • request=True
  • provider=mkl

I think the first two should really be type=interface, type=request, etc.

The main concern I have is that the proposed changes should really include a description of how each of these options affects concretization semantics and the final DAG.

Copy link
Member Author

@scheibelp scheibelp May 27, 2021

Choose a reason for hiding this comment

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

interface=True has the same concretization semantics as other variants (the difference is in terms of hashing).

Likewise provider=mkl doesn't modify this, only restricts when the variant affects the hash.

request=True doesn't modify the concretization semantics or the hash: it is only theoretically useful in terms of saving time when implementing .libs or .headers for a dependency but is not required (it doesn't have the same importance as interface=True, which is required for correct semantics).

It is true that interface=True and request=True are mutually exclusive (interface=True is used in .libs and .headers like request=True, but established additional constraints), and for that reason I suppose they could be options for a variant type. Originally I found it confusing though since their use cases are different.

Copy link
Member Author

@scheibelp scheibelp May 27, 2021

Choose a reason for hiding this comment

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

for that reason I suppose they could be options for a variant type. Originally I found it confusing though since their use cases are different.

More than that, I should say that the Boost case is simple enough that it may not need to be addressed at all here (it can definitely be addressed separately) - that section doesn't state this explicitly but I think it makes a case for this.

Copy link
Member

Choose a reason for hiding this comment

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

can you add this stuff to the SEP?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but to be clear I think this is a reasonable way to get to that: IMO it's worth making sure we agree before I push changes to this.

Copy link
Member Author

Choose a reason for hiding this comment

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

41565f3 adds some clarifications based on this conversation

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.

None yet

3 participants