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: applicable build and test fixes for conda #5093

Merged
merged 7 commits into from
Apr 13, 2023

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Apr 13, 2023

This PR includes a set of fixes that were necessary under conda, excluding the fwrite changes required from attribute_unused in the CentOS 6 sysroot.

I figured most of these will work around future problems we may hit on other one-off platforms.

grondo added 2 commits April 13, 2023 07:48
Problem: A test in libeventlog is not linked with LIBRT, so fails to
link on systems where clock_gettime is in librt.so.

Add the missing LIBRT to the test LDADD.
Problem: On some systems the link of flux-shell fails with

 local symbol `idset_encode' in libflux-internal.a is referenced by DSO

Adding libflux-idset.la explicitly in LDADD and moving libflux-internal.la
to the end seems to resolve this issue.
Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment below

Comment on lines +50 to +57
PATH=$($FLUX_BUILD_DIR/src/cmd/flux python -c \
'import os,sys; print(os.path.dirname(sys.executable))'):$PATH
Copy link
Member

Choose a reason for hiding this comment

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

perhaps note some of the corner cases I noted in #5091 that this does not fix? Or perhaps just point to the issue?

Copy link
Contributor Author

@grondo grondo Apr 13, 2023

Choose a reason for hiding this comment

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

Good point, what if we instead check that the expected condition is met? e.g.

diff --git a/t/sharness.d/01-setup.sh b/t/sharness.d/01-setup.sh
index 2af287414..08f341aeb 100644
--- a/t/sharness.d/01-setup.sh
+++ b/t/sharness.d/01-setup.sh
@@ -52,6 +52,24 @@ else # normal case, use ${top_builddir}/src/cmd/flux
     PATH=$FLUX_BUILD_DIR/src/cmd:$PATH
     fluxbin=$FLUX_BUILD_DIR/src/cmd/flux
 
+    #  Ensure executable found for "flux" matches the flux we want to test:
+    if test "$(command -v flux)" != "$fluxbin"; then
+        echo >&2 "Test environment issue:"
+        echo >&2 "flux executable $(command -v flux) does not match ${fluxbin}."
+        return 1
+    fi
+
+    #  Ensure that python3 and flux python refer to the same python
+    pybin=$(python3 -c \
+            'import os,sys; print(os.path.realpath(sys.executable))')
+    pyexpected=$(flux python -c \
+                 'import os,sys; print(os.path.realpath(sys.executable))')
+    if test "$pybin" != "$pyexpected"; then
+        echo >&2 "Test environment issue:"
+        echo >&2 "python3 ($pybin) doesn't match 'flux python' ($pyexpected)"
+        return 1
+    fi
+

Then you get (for example):

$ ./t0002-request.t 
Test environment issue:
python3 (/usr/libexec/platform-python3.6) doesn't match 'flux python' (/usr/bin/python3.8)
sharness: Error loading /g/g0/grondo/git/flux-core.git/t/sharness.d/01-setup.sh. Aborting.
FATAL: Unexpected exit with code 1

This might be a little risky because now a compatible Python might not work if it doesn't exactly match flux python 🤔

Copy link
Member

Choose a reason for hiding this comment

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

that is risky! But I think it's probably for the best. The situation I recently hit is not going to be the last time its hit, either by us developers or people just trying out flux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do a little testing and push another commit here if it works out ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, force-pushed this PR branch with an added test in sharness.d/01-setup.sh to ensure flux python matches python3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, well maybe that didn't work in CI. I'll check it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, in one builder we have two versions of Python installed that are "compatible enough" to both work for make check. I'll revert this extra check in favor of a descriptive comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, force-pushed removal of that failed experiment and added a verbose comment to 01-setup.sh.

Copy link
Member

Choose a reason for hiding this comment

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

comment looks good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll set MWP assuming this revision passes (again)

grondo added 3 commits April 13, 2023 13:56
Problem: Some source files need to include signal.h on older glibc
(2.17).

Add the missing includes of signal.h.
Problem: libterminus fails to compile on older versions of libc that
do not define the TIOCSIG ioctl.

Use of this ioctl to signal all processes using the pty is not
essential, so just conditionally use it based on presence of TIOCSIG.
Problem: Not all build systems have a man(1) executable available.
This causes `flux help` tests in t0001-basic.t to fail.

Check for a man executable and set a HAVE_MAN prereq if found. Require
the HAVE_MAN prereq on tests that assume presence of man(1).
grondo added 2 commits April 13, 2023 14:34
Problem: If flux is configured with a non-default python3, then some
test scripts that use `#!/usr/bin/env python3` may fail since the
default python3 in PATH is not the version configured with flux.

Prepend the path to the `flux python` executable in PATH so it is found
before any other python3. Then prepend $FLUX_BUILD_DIR/src/cmd to PATH
so that the built flux(1) executable is found before any flux that shares
a path with `flux python`.
Problem: If t9000-system.t is run with -d, --debug, or the debug
environment variable is set, _and_ no flux is installed in /usr/bin,
then the test fails with a message:

 Failed to find a flux binary in /usr/bin/flux.
 Do you need to run make?

Allow the test to be run with -d, --debug, but only set
FLUX_TEST_INSTALLED_PATH to /usr/bin if an executable exists in
that path.
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #5093 (353ebd2) into master (d5572ff) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 353ebd2 differs from pull request most recent head 5282edd. Consider uploading reports for the commit 5282edd to get more accurate results

@@           Coverage Diff           @@
##           master    #5093   +/-   ##
=======================================
  Coverage   83.13%   83.13%           
=======================================
  Files         445      445           
  Lines       76628    76628           
=======================================
+ Hits        63701    63706    +5     
+ Misses      12927    12922    -5     
Impacted Files Coverage Δ
src/broker/state_machine.c 80.69% <ø> (ø)
src/cmd/builtin/proxy.c 79.57% <ø> (+2.12%) ⬆️
src/common/libsubprocess/server.c 78.93% <ø> (ø)
src/common/libterminus/pty.c 88.10% <ø> (ø)

... and 10 files with indirect coverage changes

@mergify mergify bot merged commit d29c855 into flux-framework:master Apr 13, 2023
@grondo grondo deleted the conda-build branch April 13, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants