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

"configure" script for platform detection #611

Merged
merged 3 commits into from
Mar 27, 2020

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Mar 25, 2020

Move most of the platform detection logic out from the Makefile into a traditional configure script, Since the system is unlike to change, this allows to run (costly) platform detection things only once at the first build. Incremental builds during development become much faster.

macOSLinux
BeforeAfterBeforeAfter
make clean in empty dir 3.196 2.858 0.890 1.166
make all from scratch 11.762 12.183 6.226 6.273
make all again with no changes 3.231 0.504 0.936 0.300
make all with one changed file 3.247 0.816 1.101 0.553
make clean after build 4.564 0.939 0.896 0.328

After a while you get really annoyed by every make operation taking usually around three seconds and up to 8 (eight, Carl!) seconds if caches are cold. Lowering it to sub-second range is more tolerable.

Why so slow?

The main culprit here is Homebrew that really takes its time to locate OpenSSL installation directory. Thankfully, it does not update the formula database now, but it did before. Some people complained about this simple operation taking enormous amount of time but the maintainer said this is fine and not a bug.

All other scripting languages combined can be treated as accomplices. Each individual version check looks cheap on its own, but doing them altogether takes a couple of seconds.

Surprisingly, the supported calls to check C compiler flags are fast enough so I did not bother moving them out to ./configure (though it's a task traditionally performed by it). There's also some weird interplay with AFL there, so that's left for the future.

On ./configure

The script follows GNU conventions expected from it though there is not much configuration involved. We also diverge from the usual approach to out-of-source builds. The traditional way is to call configure from the build directory with --srcdir, but we keep using BUILD_PATH to set
the build directory and expect make to be call from tree root.

Note that it is not necessary to explicitly call ./configure before the build. Make will check if it has not been called and will configure the build if necessary. So for most of the users the happy path is still

make
sudo make install

But you can use ./configure if, say, you want a different installation prefix:

./configure --prefix=/opt/themis
make
sudo make install

The entire script is an exercise in portable POSIX shell scripting. Don't you dare utter "Au*****ls" in this house.

What are those fancy comment sections?

We're (slowly) unifying the makefile structure across projects. This is the new way™ of doing things.

Checklist

  • Change is covered by automated tests
  • Benchmark results are attached (if applicable)
  • The coding guidelines are followed
  • Public API has proper documentation

Move most of the platform detection logic out from the Makefile into
a traditional "configure" script, Since the system is unlike to change,
this allows to run (costly) platform detection things only once at the
first build. Incremental builds during development become much faster.

                                       macOS             Linux
                                  Before   After    Before  After
  make clean in empty dir          3.196   2.858     0.890  1.166
  make all from scratch           11.762  12.183     6.226  6.273
  make all again with no changes   3.231   0.504     0.936  0.300
  make all with one changed file   3.247   0.816     1.101  0.553
  make clean after build           4.564   0.939     0.896  0.328

After a while you get *really* annoyed by every make operation taking
usually around three seconds and up to 8 (eight, Carl!) seconds if
caches are cold. Lowering it to sub-second range is more tolerable.

Why so slow?
------------

The main culprit here is Homebrew that really takes its time to locate
OpenSSL installation directory. Thankfully, it does not update the
formula database now, but it did before. Some people complained [1]
about this simple operation taking enormous amount of time but the
maintainer said this is fine and not a bug.

[1]: Homebrew/brew#3097

All other scripting languages combined can be treated as accomplices.
Each individual version check looks cheap on its own, but doing them
altogether takes a couple of seconds.

Surprisingly, the "supported" calls to check C compiler flags are fast
enough so I did not bother moving them out to ./configure (though it's
a task traditionally performed by it). There's also some weird interplay
with AFL there, so that's left for the future.

On "./configure"
----------------

The script follows GNU conventions expected from it [2] though there is
not much configuration involved. We also diverge from the usual approach
to out-of-source builds. The traditional way is to call configure from
the build directory with --srcdir, but we keep using BUILD_PATH to set
the build directory and expect "make" to be call from tree root.

[2]: https://www.gnu.org/prep/standards/html_node/Configuration.html

Note that it is not necessary to explicitly call ./configure before the
build. "make" will check if it has not been called and will configure
the build if necessary. So for most of the users the happy path is still

    make
    sudo make install

But you can use ./configure if, say, you want a different installation
prefix:

    ./configure --prefix=/opt/themis
    make
    sudo make install

The entire script is an exercise in portable POSIX shell scripting.
Don't you dare utter "Au*****ls" in this house.

What are those fancy comment sections?
--------------------------------------

We're (slowly) unifying the makefile structure across projects.
This is the new way™ of doing things.
@ilammy ilammy added the infrastructure Automated building and packaging label Mar 25, 2020
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

lgtm

--*=*)
var=${1#--}
value=${var#*=}
var=${var%%=*}
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, that is why I hate bash and makefile languages... too much non-alpha symbols in syntax

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You'll enjoy APL then 😉

{↑1 ⍵∨.∧3 4=+/,¯1 0 1∘.⊖¯1 0 1∘.⌽⊂⍵}

@ilammy
Copy link
Collaborator Author

ilammy commented Mar 25, 2020

Okay, no clue why PHP build broke, but it seems that something is wrong with PHP 5.6, not this changeset, and it's not some transient issue in GitHub Actions.

Linux)
set_variable IS_LINUX "true"
;;
*)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest:

  • to specify explicitly pattern for MS: MINGW*|CYGWIN*|MSYS*)
  • to throw an exception on unknown platforms

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

specify explicitly pattern for MS

IIRC, this one is specifically for MSYS2, not a generic Windows platform. Windows support is in kinda "purgatory" state, since it's not tested automatically, and not regularly tested manually either. So I'd be wary to add new behavior without proper testing.

I'd suggest to keep the current behavior (detect only MSYS2) and add other platforms later, in a more systematic way.

throw an exception on unknown platforms

Hm... Seems like a good idea, but it may break some builds. Currently, if we cannot detect the platform we assume it to be generic UNIX system. This way make will probably work on various BSDs, etc. If we disallow unknown systems then we might break builds for some users that build Themis for platforms that we don't explicitly support yet they still work.

Are we okay with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. Thanks.

Since GitHub Actions for PHP install all PHP versions and switch between
them for tests, we need to run ./configure after every switch to apply
the current default PHP version to the configuration.
@ilammy
Copy link
Collaborator Author

ilammy commented Mar 26, 2020

I have realized why PHP build on GHA broke with PHP 5.6 while CircleCI build is fine.

This is because with these changes we no longer detect environment changes on each make. The whole point of these changes is to avoid this behavior. However, this means that if the environment changes between builds then a manual ./configure (or make clean) is necessary to apply the updates.

GHA testing script installs all PHP versions and switches them during the test run. However, PHP 7 is the default one and that's what we detect on the first make call. After we switch to PHP 5.6 being default, the configuration file still says that we're using PHP 7.

CircleCI runs PHP tests in isolated environments so PHP versions gets detected there correctly.

Well, we can't do much about it as this will be the expected behavior with these changes. It should not affect non-developers as they are unlikely to change environments mid-build. However, we need to be more careful in the future. All hail the tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Automated building and packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants