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

Add libs multivalued variants to lzo, lz4, xz, binutils #23474

Merged
merged 4 commits into from
May 12, 2021

Conversation

haampie
Copy link
Member

@haampie haampie commented May 6, 2021

No description provided.

@haampie haampie changed the title Add static/shared variants to lzo and lz4 Add static/shared variants to lzo, lz4, xz May 6, 2021
eugeneswalker
eugeneswalker previously approved these changes May 6, 2021
Copy link
Contributor

@eugeneswalker eugeneswalker left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 29 to 30
variant('shared', default=True, description='Build shared library')
variant('static', default=True, description='Build static library')
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not convinced of this modeling as lz4~shared~static doesn't make much sense. Can we use either a multi-valued variant or just shared as many packages do (and assume ~shared is equivalent to ask for static)?

Copy link
Member Author

@haampie haampie May 7, 2021

Choose a reason for hiding this comment

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

hm, it maps directly to the autotools flags --[enable|disable]-static and --[enable|disable]-shared.

Copy link
Member

Choose a reason for hiding this comment

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

For example:

$ spack install lzo~shared~static
[ ... ]
$ tree /home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu18.04-broadwell/gcc-10.3.0/lzo-2.10-q3odxvlthiprpuxgv426djmokueg2ulz
/home/culpo/PycharmProjects/spack/opt/spack/linux-ubuntu18.04-broadwell/gcc-10.3.0/lzo-2.10-q3odxvlthiprpuxgv426djmokueg2ulz
├── include
│   └── lzo
│       ├── lzo1a.h
│       ├── lzo1b.h
│       ├── lzo1c.h
│       ├── lzo1f.h
│       ├── lzo1.h
│       ├── lzo1x.h
│       ├── lzo1y.h
│       ├── lzo1z.h
│       ├── lzo2a.h
│       ├── lzo_asm.h
│       ├── lzoconf.h
│       ├── lzodefs.h
│       └── lzoutil.h
├── lib
│   ├── liblzo2.a
│   └── pkgconfig
│       └── lzo2.pc
└── share
    └── doc
        └── lzo
            ├── AUTHORS
            ├── COPYING
            ├── LZOAPI.TXT
            ├── LZO.FAQ
            ├── LZO.TXT
            ├── NEWS
            └── THANKS

This doesn't seem consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually i think it's better to have 2 flags, so that packages can always to depends_on('lz4 +shared') if they need shared libs, and another can do depends_on('lz4 +static'), and it won't lead to conflicts, whereas depends_on('lz4 ~shared') would be a concretization failure.

Copy link
Member

Choose a reason for hiding this comment

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

and it won't lead to conflicts, whereas depends_on('lz4 ~shared') would be a concretization failure.

Yes, but they would need to conflict with ~shared~static. I think the best way to model what you say is a multi-valued variant.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alalazo the github ui hadn't added your reply inbetween my 2 replies :p I agree that it's not entirely consistent

@mwkrentel
Copy link
Member

@alalazo I'm afraid I must disagree. Suppose I use package xz and
shared liblzma.so. That is, I need shared, I don't care whether
static exists or not.

Suppose there's a 3-valued variant.

variant('libs', default='shared', values=('shared', 'static', both'), multi=False)

How do I write depends_on? If I say depends_on('xz libs=shared'),
then I'm incompatible with any package that requires static. If I say
depends_on('xz libs=both'), then we're right back where we are now.
What I want to say is the following, but this doesn't exist in spack.

depends_on('xz libs=shared OR libs=both')

So, you have to drop both and turn on multi=True.

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

Now I can say depends_on('xz libs=shared') and someone can override
or extend me with ^xz libs=static and get both.

But what does this accomplish? This is really just a complicated version
of the original +shared +static proposal.

The advantage of separate +shared +static is that I can say that I
need shared without putting any constraints on static.

@alalazo
Copy link
Member

alalazo commented May 8, 2021

@mwkrentel What you say above is a single-valued variant, what I was proposing is a multi-valued variant:

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

If you say:

depends_on('xz libs=shared')

it will match both xz libs=shared or xz libs=shared,static.

@mwkrentel
Copy link
Member

Is there any advantage of the multi-valued variant?

The separate shared/static variants are simple, widely used and
well-understood. IMO the multi-valued case is just a complicated way
of saying the same thing.

@alalazo
Copy link
Member

alalazo commented May 8, 2021

@mwkrentel With 2 boolean variants we have 4 choices. What would be the meaning of ~shared~static?

@alalazo
Copy link
Member

alalazo commented May 8, 2021

The separate shared/static variants are simple, widely used and well-understood.

I don't think they are really widely used in the same package (which is my point, most packages have only one of the two and behave as the other is always on). @haampie Besides theoretical discussions on modelling, what would be the disadvantage in this PR if we build both but don't add variants?

@alalazo
Copy link
Member

alalazo commented May 8, 2021

Linking #5269 since the topic was really opinionated also years ago 😆

@mwkrentel
Copy link
Member

What would be the meaning of ~shared~static?

Probably the same as libs=() (or however you specify empty list for
multi valued), ie, Pilot Error.

Anyway, this is a distraction. If it's a problem, then add a conflict.

I don't think they are really widely used in the same package (which is my point, most packages have only one of the two and behave as the other is always on).

However common it is to use both +shared and +static in the same
package, their meaning is very clear.

I still don't see the added value of packaging them into a list.

I think one of the problems in #5269 is that it doesn't consider the
case of overlapping prereqs. Package A wants foo +shared, package B
wants foo +shared. Rather than building it twice, I could build it
once with both static and shared.

Anyway, if this has been discussed for 4 years without a clear
directive that doesn't break some use case, then I don't think it's
fair to blame this PR.

I was just arguing that separate variants is simpler than a list. For
example, libs=shared isn't all that clear if it means add shared to
the list, or set the entire list to just shared. I had to run tests
to see what this meant.

Personally, I'm fine with always building both shared and static. The
static libs aren't all that big (1.5 Meg) for xz.

@alalazo
Copy link
Member

alalazo commented May 9, 2021

Probably the same as libs=() (or however you specify empty list for multi valued)

You can't specify that, that's the point for using multi-valued variant. Sometimes ago, for cases where an empty set had to be used, we started giving that meaning to the string none.

@alalazo
Copy link
Member

alalazo commented May 9, 2021

Personally, I'm fine with always building both shared and static. The static libs aren't all that big (1.5 Meg) for xz.

That would be a practical solution for this specific PR, if possible. Don't add variants but always build both.

@haampie
Copy link
Member Author

haampie commented May 9, 2021

I want to be able to build static libs only s.t. I can easily build tiny executables for squashfuse and the appimage runtime (~100kb). If there are shared and static libs it's more work to make the compiler pick the static lib.

@haampie
Copy link
Member Author

haampie commented May 9, 2021

Some more thoughts:

(1)

  • some people (like me) may want to have tiny binaries and only build static libs
  • some people may want to have shared libs only because of GPL licensing or so, and they don't want to accidentally link statically
  • having both shared & static libs can be useful too to accomodate for both, but in practice I feel like you don't really control what other packages actually pick up -- e.g. you'd have to add compiler flags -Wl,-Bstatic -lfoo -Wl,-Bdynamic, but thay may require a patch to the package.

So, just shared is not enough.

(2)

The upside of shared and static is that you can depend on a negative; e.g. if you insist on shared libs because of licensing issues or so you could depend on ~static+shared, whereas you can't really express ~static explicitly using a multivalued variant, right?

(3)
In the libs function spack assumes +shared means shared and ~shared means static and no shared/static variant implies looking for both shared and static.

(4)
The mesonpackage has this variant called default_library=[static|shared|both], which directly maps to a meson option. I don't think both is a good choice in spack-language... we could change it to +shared+static or libs=shared,static and then do the mapping to this flag with -default_library=both ourselves. That way we don't get concretizer issues when one package needs default_library=shared and another default_library=both.

@alalazo
Copy link
Member

alalazo commented May 9, 2021

The upside of shared and static is that you can depend on a negative; e.g. if you insist on shared libs because of licensing issues or so you could depend on ~static+shared, whereas you can't really express ~static explicitly using a multivalued variant, right?

A case like:

depends_on('libfoo +static~shared')

with boolean variants translates to:

depends_on('libfoo libs=static')
conflicts('libfoo libs=shared')

with multi-valued variants. The only downside for more complex variants is that you can't express easily, in the package declaring the mv variant itself, that if one value is in then another must be in too:

# This can't be done at the moment
conflicts('libs=A', when='libs= not B')

in general we lack a way to express the absence of a value in a directive but, if I am not overlooking anything, that shouldn't matter in this use case.

@mwkrentel
Copy link
Member

@alalazo So, what is the syntax for removing an item from a
multi-valued variant? Is this possible?

@haampie If what you really want is to build without shared, then you
could drop the static variant and just add the shared variant.
So, it would always build static and optionally build shared.

But I caution you, I think it's a bad idea in general to require that
something not be included. You want ~shared, but someone else will
require +shared and then you have to build the package twice. In
this case, xz is small, but there could be other cases of a larger
package that you're require to be built twice.

In this case, if your build system insists on using shared over static
and there is no option to force static, then I'd say your build system
is broken and that's what you should fix instead of xz.

What package is this?

@alalazo
Copy link
Member

alalazo commented May 10, 2021

@mwkrentel

So, what is the syntax for removing an item from a multi-valued variant? Is this possible?

It's not clear to me what "removing" means in this context. Do you mean preventing a value to be in a concretized spec? In that case you can use a conflicts directive as sketched in #23474 (comment)

@haampie
Copy link
Member Author

haampie commented May 10, 2021

So, at the end of the day I would be a fan of libs=static, but right now it's a bit awkward to use from the command line:

spack install xz libs=static

will not override the default but extend it, so if the default is libs=shared,static or libs=shared, it'll concretize to libs=shared,static instead of libs=static. Until that is solved in some way, I'd rather just use spack install xz ~shared +static.

@haampie haampie changed the title Add static/shared variants to lzo, lz4, xz Add static/shared variants to lzo, lz4, xz, binutils May 10, 2021
@alalazo
Copy link
Member

alalazo commented May 10, 2021

Until that is solved in some way, I'd rather just use spack install xz ~shared +static.

Issue will be solved in #23542

@haampie haampie changed the title Add static/shared variants to lzo, lz4, xz, binutils Add libs multivalued variants to lzo, lz4, xz, binutils May 10, 2021
@mwkrentel
Copy link
Member

I guess I'm still at a loss to identify a use case where the
multi-valued variant is superior (more expressive, easier to
understand semantics, etc) to the simpler +/-shared,static.

But it doesn't block me from building xz the way I need it,
at least not yet, so whatever.

@haampie
Copy link
Member Author

haampie commented May 12, 2021

The upside is mainly there's no need to add a conflict for ~shared ~static as only libs=static libs=shared libs=static,shared are valid options.

@alalazo alalazo merged commit b768d7b into spack:develop May 12, 2021
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this pull request Jun 9, 2021
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

4 participants