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

Instruct Ruby to fail the build if openssl or psych are missing #2296

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

mislav
Copy link
Member

@mislav mislav commented Nov 7, 2023

Normally, Ruby make step will print a warning about any missing extensions, but will not abort the build and instead proceed as normal.

Since Ruby installations without openssl or psych are essentially broken, ruby-build used to have a verify_openssl build step to test if the newly built Ruby can load these extensions, and print helpful information and abort the build on errors:

Loading the Ruby openssl extension failed
ERROR: Ruby install aborted due to missing extensions

The verify_opensl implementation was necessary to provide a good experience for ruby-build users, but was hacky and I would prefer to eliminate it. Fixes #2241, fixes #1415

It appears that passing --with-ext=openssl,psych to the Ruby configure step marks those extensions as mandatory and fails the make process if they failed to build. That is exactly the behavior we want, so this change enables the configure option for all Ruby builds. ruby/ruby@b58a30e

*** Following extensions are not compiled:
openssl:
	Could not be configured. It will not be installed.
	/private/var/folders/6k/fwzzx67n163b0lfftyw3313h0000gn/T/ruby-build.20231107113317.26428.4N23Wf/ruby-3.2.2/ext/openssl/extconf.rb:101: OpenSSL library could not be found. You might want to use --with-openssl-dir=<dir> option to specify the prefix where OpenSSL is installed.
	Check ext/openssl/mkmf.log for more details.
psych:
	Could not be configured. It will not be installed.
	Check ext/psych/mkmf.log for more details.
*** Fix the problems, then remove these directories and try again if you want.
make[1]: *** [note] Error 1
make: *** [build-ext] Error 2

/cc @nobu @hsbt: as people who are more familiar with Ruby build process than myself, do you foresee any potential problems with using ./configure --with-ext=openssl,psych by default when building any Ruby version, even old ones? From what I can see, the --with-ext flag seems to have existed for a long time, but only started having the behavior that we want from Ruby 2.5.0. Thank you!

The new default ruby-build behavior can be avoided by explicitly passing another --with-ext value, e.g.:

ruby-build 3.2.2 /tmp/ruby-3.2.2 -- --with-ext=+

@eregon
Copy link
Member

eregon commented Nov 7, 2023

I think given this is a flag that basically no end user would use and so probably receives little testing I would suggest to avoid using it in ruby-build, it's not so unlikely to cause issues.

For instance what if --with-ext=openssl,psych changed to mean only build those 2 exts and no other exts?

Verifying if openssl/psych is there after building is trivial and portable, that incurs no risks.

@mislav
Copy link
Member Author

mislav commented Nov 7, 2023

Verifying if openssl/psych is there after building is trivial and portable, that incurs no risks.

Good point about there being no guarantee that --with-ext will work consistently. However, for reasons mentioned above, it's not like verify_openssl was trivial by comparison. It required maintaining an inline Ruby script, it ran too late in the build process (after make completed and make install was over), and it sabotages installs for people compiling with the DESTDIR setting. I would rather rely more on Ruby's own build implementation than maintain workarounds for this within ruby-build.

@eregon
Copy link
Member

eregon commented Nov 7, 2023

Could we check the logs for things like:

openssl:
	Could not be configured. It will not be installed.

It would likely work quite well on all Ruby versions.

Or maybe even simpler/safer, to automatically scan for these lines and print them highlighted in the ruby-build output? So users can easily see which extensions have been skipped.

@eregon
Copy link
Member

eregon commented Nov 7, 2023

Well at least that feature seems explicitly supported since https://bugs.ruby-lang.org/issues/13302, so it seems not so risky.
What does the flag do though on < 2.5?

@mislav
Copy link
Member Author

mislav commented Nov 7, 2023

Could we check the logs for things like:

openssl:
	Could not be configured. It will not be installed.

That was my initial approach to solving this, before I discovered --with-ext. There are some hurdles that I've had with scanning the log file:

  • Ruby build system seems to output ANSI colors for these "Could not be configured" reports even when output goes to a pipe and when NO_COLOR is set. This seems to be fixed in Ruby 3.3.0-dev, but for older Ruby versions we would have to first strip ANSI sequences before pattern-matching.

  • In ruby-build --verbose mode, the stderr and stdout of make command (as well as any other external command) is connected directly to the user's terminal, as there is no log file. This makes it harder to scan for particular output.

  • Scanning the output for text following the "Following extensions are not compiled" section is also its own kind of brittle, since any regular expressions matching this output would be made obsolete if someone tweaked this text a bit in Ruby core. I'm having more confidence that a ./configure flag would be more stable in the longer run than the make output format.

@mislav
Copy link
Member Author

mislav commented Nov 7, 2023

What does the flag do though on < 2.5?

I'm not sure! That is why I cc'ed some Ruby core folks above to ask for advice. 🙇

I'm hoping that it's simply ignored, since that would be preferable for me. If an unrecognized flag causes build failures, that would be a dealbreaker (even for Ruby versions that are so old that they are EOL).

Base automatically changed from output-redesign to master November 7, 2023 14:49
Normally, Ruby `make` step will print a warning about any missing
extensions, but will not abort the build and instead proceed as normal.

Since Ruby installations without openssl or psych are essentially
broken, ruby-build used to have a `verify_openssl` build step to test if
the newly built Ruby can load these extensions, and print helpful
information and abort the build on errors:

    Loading the Ruby openssl extension failed
    ERROR: Ruby install aborted due to missing extensions

The `verify_opensl` implementation was necessary to provide a good
experience for ruby-build users, but was hacky and I would prefer to
eliminate it.

It appears that passing `--with-ext=openssl,psych` to the Ruby configure
step marks those extensions as mandatory and fails the `make` process if
they failed to build. This is exactly the behavior we want, so this
enables the configure option for all Ruby builds.
The functionality to fail the build if openssl or psych are missing is
now built into the `./configure && make` phase for Ruby.
A very common type of build failure is that the Ruby "openssl" extension failed to compile. However, when that happens, ruby-build just prints a generic "BUILD FAILED" message. To find out what happened, the user would first have to open the ruby-build log, note the "Following extensions are not compiled" section, then figure out how to find the exact location to the "ext/openssl/mkmf.log" file for more information.

Now, when `make` fails, ruby-build will automatically forward the "Following extensions are not compiled" information to stderr.
@mislav
Copy link
Member Author

mislav commented Nov 21, 2023

I have updated this changeset into something shippable:

  • I am now using the --with-ext=openssl,psych,+ syntax (notice the plus character). This ensures that all the default extensions are still getting built even though --with-ext was passed. The "openssl" and "psych" extensions are still marked as "mandatory".

  • "Following extensions are not compiled" section from the make log is now forwarded to stderr so that the user can spot the extremely common type of OpenSSL problems more directly:

@mislav
Copy link
Member Author

mislav commented Nov 21, 2023

Adding another comment to bust GitHub's frontend cache. I am positive there has been timeline items on this thread newer than 2 days ago, but GitHub isn't showing it even after refreshes.

@mislav mislav merged commit e30482e into master Nov 21, 2023
6 checks passed
@mislav mislav deleted the verify-openssl branch November 21, 2023 17:19
@mislav
Copy link
Member Author

mislav commented Nov 21, 2023

@eregon Thank you for the in-depth reflection on this PR. I went back and ascertained that using --with-ext should be safe for all Ruby versions. I have also added some rudimentary log file scanning, but that is only done to surface errors better and not to fail the build based on the contents of the log, which I feel would be brittle.

I am very pleased that we now git rid of the long inline Ruby script that did the job of a single Ruby configuration flag.

@anevsevra
Copy link

It breaks build for at least Ruby 2.4.10. Switching back to v20231114 allows to build 2.4.10 again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants