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

build: Improve the configure experience #1429

Closed

Conversation

jbergstroem
Copy link
Member

Not sure about the PR topic here, but a few - imo - important changes in here, mainly concerning shared builds:

  • All handling of headers/libraries/etc for a shared library is only processed if a shared build is enabled (--shared-foo)
  • I incorrectly handled return from b() in a previous commit. We now compare strings instead.
  • Move shared and i18n flags into their own option groups with a short description of each
  • i18n --with-intl is now validated the same way other choice options are (optparse)
  • better integrate with pkg-config if shared builds are enabled and let users override pkg-config path via the environment (useful in cross-compilation environments, found in a floating patch)

Open to further suggestions to improve the language of configure which is decent at best. Also, let me know if I should squash (and how).

/R=@iojs/build, ... ?

Edit: Adding R=@bnoordhuis since he regularly touches build/configure.

Move all logic for headers/libraries for each shared library
into the shared library scope - similar to how OpenSSL already works.

Also, don't assume that b() returns a boolean.
Attempt to fetch information from pkg-config if available.
Also, let the user provide a custom PKG_CONFIG path, based on a
floating patch by Paul McClave <pmcclave@chromium.org>.

Additionally, make library input parsing more robust as well as
deduplicating what we provide to the linker.
Minor edits to current build flags and its help texts as well
as grouping shared and i18n options into separate option groups.

Hopefully this will improve the readability of `./configure` output.
Avoid custom validation as well as simplify output generated
by configure. Add 'system-icu' to the list of valid choices.
@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Apr 15, 2015
@jbergstroem
Copy link
Member Author

In the last @iojs/build meeting we talked about potentially checking bundled versions with pkg-config. I think it's a good thing since it helps us in cases where we assume some versions being available (we will soon depend on openssl 1.0.2-specific stuff and we already assume libuv is at least 1.0).

The biggest downside of this would be the complexity added to configure for parsing version out of bundled deps. I don't really see adding static versions somewhere as an alternative.

How does others feel about this?

@monsanto
Copy link
Contributor

Is there some reason for writing if b(expr) == 'true': instead of just if expr:?

@jbergstroem
Copy link
Member Author

@monsanto since b() always returns a string (its first usage was probably to return a gyp-friendly string to put into our generated config.gypi), python will treat that statement as boolean true:

>>> foo = 'false'
>>> if foo:
...   print 'true after all'
... 
true after all
>>> if foo == 'true':
...   print 'you\'ll never see me'
...

@monsanto
Copy link
Contributor

@jbergstroem I don't understand what your point is. if b(expr) == 'true': and if expr: are exactly the same by the definition of b(), the only reason to prefer the former is some weird in-house style (which is why I'm asking).

@jbergstroem
Copy link
Member Author

@monsanto sorry, I misunderstood the comment. Your suggestion is correct -- just pushed a new version.

Edit: simplified my reply (no need to elaborate on reasoning).

@rvagg
Copy link
Member

rvagg commented Apr 16, 2015

lgtm, might be good to get sign-off from @bnoordhuis too

@Fishrock123 Fishrock123 added this to the 1.8.0 milestone Apr 16, 2015
@Fishrock123 Fishrock123 removed this from the 1.8.0 milestone Apr 17, 2015
@jbergstroem
Copy link
Member Author

So, current state:

  • drop parts of 867de71 since it already landed
  • @bnoordhuis wants --shared-whatever-includes to override --shared-whatever (--shared-whatever-libraries is already set by default) as convenience. I'm not very keen on this but open to discuss.
  • I'll do some more work on cases that should fail, such as mixing shared and static flags.
  • I'd like to better fence around what we assume as default from pkg-config -- even if pkg-config will likely provide more accurate information, it would just overwrite the user input based on this patchset (and already does for openssl in master/v1.x)

@jbergstroem
Copy link
Member Author

Ima close this and split it up into different PR's instead. Easier to review and discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants