Opened 7 years ago

Closed 5 years ago

#37844 closed submission (fixed)

Port submission: leveldb

Reported by: dw@… Owned by: macports-tickets@…
Priority: Normal Milestone:
Component: ports Version:
Keywords: Cc: daniel@…
Port: leveldb

Description (last modified by larryv (Lawrence Velázquez))

Per #37843, snappy +universal fails to build if google-test -universal is installed, so +universal +snappy builds of this Portfile will fail similarly.

Attachments (2)

Portfile (1.3 KB) - added by dw@… 7 years ago.
Updated Portfile that doesn't install includes to ${prefix}/include/include
Portfile.2 (1.1 KB) - added by dw@… 7 years ago.
Portfile incorporating ryandesign's comments

Download all attachments as: .zip

Change History (8)

comment:1 Changed 7 years ago by larryv (Lawrence Velázquez)

Description: modified (diff)
Summary: New Portfile for Google leveldb.Port submission: leveldb
Type: enhancementsubmission
Version: 2.1.2

Thanks for the submission.

Changed 7 years ago by dw@…

Attachment: Portfile added

Updated Portfile that doesn't install includes to ${prefix}/include/include

comment:2 Changed 7 years ago by ryandesign (Ryan Schmidt)

Thanks. Some comments:

  • The distname line can be removed; that's the default.
  • "configure {}" should be replaced with "use_configure no"; see "port lint"
  • You're hardcoding the list of architectures to use in the universal variant, and not setting any architecture when not using the universal variant; you need to always set architectures, and use the list of architectures the user requested. The [get_canonical_archflags] procedure is useful for this. Ensure you define at least an empty universal variant ("variant universal {}") before invoking [get_canonical_archflags] (and in this portfile I don't think you'll need more than an empty universal variant).
  • In the universal variant, the second build.env statement overrides the first. If you want to append to the environment rather than overwriting it, use build.env-append, but note that you can set multiple environment variables in a single build.env invocation, which is usually preferable.
  • The snappy variant only adds a dependency; it does nothing to ensure that snappy will not be used if the snappy variant is not selected. Is there a reason why snappy needs to be a variant? Couldn't snappy support just be enabled all the time?
  • In destroot, you don't need to create the lib directory; MacPorts automatically creates all the directories of the standard mtree for you.
  • What's the "platform darwin" block all about? Why is INSTALL_PATH set to ${prefix}/lib only on Darwin? What will happen on other OSes?
  • +universal should usually not be a default variant, unless there are extenuating circumstances.

comment:3 in reply to:  2 Changed 7 years ago by dw@…

Thanks for taking the time to reply to a noob.. :P~

  • The distname line can be removed; that's the default.
  • "configure {}" should be replaced with "use_configure no"; see "port lint"

Fixed.

  • You're hardcoding the list of architectures to use in the universal variant, and not setting any architecture when not using the universal variant; you need to always set architectures, and use the list of architectures the user requested. The [get_canonical_archflags] procedure is useful for this. Ensure you define at least an empty universal variant ("variant universal {}") before invoking [get_canonical_archflags] (and in this portfile I don't think you'll need more than an empty universal variant).
  • In the universal variant, the second build.env statement overrides the first. If you want to append to the environment rather than overwriting it, use build.env-append, but note that you can set multiple environment variables in a single build.env invocation, which is usually preferable.

Now there is a single build.env-append statement and the universal variant is defined.

  • The snappy variant only adds a dependency; it does nothing to ensure that snappy will not be used if the snappy variant is not selected. Is there a reason why snappy needs to be a variant? Couldn't snappy support just be enabled all the time?

In this case, leveldb's build_detect_platform notices the libraries and enables them, so all required is to ensure they exist. I concur that just removing the variant makes more sense: snappy is a motivating feature of LevelDB and it can still be disabled at runtime. The alternative is battling the simplistic build_detect_platform into ignoring it.

  • In destroot, you don't need to create the lib directory; MacPorts automatically creates all the directories of the standard mtree for you.

Removed.

  • What's the "platform darwin" block all about? Why is INSTALL_PATH set to ${prefix}/lib only on Darwin? What will happen on other OSes?

Blindly copying examples from other Portfiles. :( I've moved it to the single remaining build.end-append statement. It controls -rpath via build_detect_platform.

  • +universal should usually not be a default variant, unless there are extenuating circumstances.

Fixed.

Please find a revised Portfile attached.

Last edited 7 years ago by dw@… (previous) (diff)

Changed 7 years ago by dw@…

Attachment: Portfile.2 added

Portfile incorporating ryandesign's comments

comment:4 Changed 6 years ago by daniel@…

Cc: daniel@… added

Cc Me!

comment:5 Changed 6 years ago by dw@…

Hi,

It's been nearly a year since I spent almost a full day packaging LevelDB for MacPorts, so I wouldn't have to waste time maintaining manual hacks to get it installed. 9 months later and it's starting to look like that day was wasted. Note that other competing packaging systems already provide LevelDB – I've done all the legwork required for MacPorts, and now I'm starting to consider alternatives. Why hasn't this been applied yet?

comment:6 Changed 5 years ago by mf2k (Frank Schima)

Port: leveldb added
Resolution: fixed
Status: newclosed

Sorry for the delay. Committed in r119873.

Keep in mind we are all volunteers and we are all busy.

Note: See TracTickets for help on using tickets.