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

scylla_node: populate env variable when running scylla-sstable #502

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

tchaikov
Copy link
Contributor

if scylla is built in a dtest environment, and the shared libraries which it is linked against cannot be found in the testbed's default ld.so.conf paths, the tests which run scylla-sstable would fail.

so, in this change. let's apply the self._launch_env when running scylla-sstable as well. previously, these env variables are only applied when launching scylla as a daemon.

if scylla is built in a dtest environment, and the shared libraries
which it is linked against cannot be found in the testbed's default ld.so.conf
paths, the tests which run scylla-sstable would fail.

so, in this change. let's apply the `self._launch_env` when
running scylla-sstable as well. previously, these env variables
are only applied when launching scylla as a daemon.

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov tchaikov marked this pull request as draft August 29, 2023 11:28
@tchaikov
Copy link
Contributor Author

only for testing the CCM CI test using Nix.

@tchaikov
Copy link
Contributor Author

tested using

$ scripts/dbuild_collect_so.sh ~/dev/scylladb/build/cmake/scylla /tmp/dynamic_libs_for_dtest
$ SCYLLA_DBUILD_SO_DIR=/tmp/dynamic_libs_for_dtest JAVA_HOME=/usr/lib/jvm/jre-11 pytest --cassandra-dir $HOME/dev/scylladb/build/cmake scylla_sstable_test.py::TestScyllaSstableDumpData::test_scylla_sstable_basic

not really a dbuild build. but i also verified by checking the env variables manually.

Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

@fruch
Copy link
Contributor

fruch commented Aug 29, 2023

I'll for @bhalevy to confirm, as the one that was hit by this issue, to validate it's solving it

@tchaikov
Copy link
Contributor Author

tchaikov commented Aug 29, 2023

I'll for @bhalevy to confirm, as the one that was hit by this issue, to validate it's solving it

@fruch Israel, Benny approved a superset of this change. see #501. but it introduced a regression identified by the ccm's own unit tests. so i abandoned it.

@bhalevy
Copy link
Member

bhalevy commented Aug 29, 2023

if scylla is built in a dtest environment,

nit: s/dtest/dbuild/

and the shared libraries which it is linked against cannot be found in the testbed's default ld.so.conf paths, the tests which run scylla-sstable would fail.

so, in this change. let's apply the self._launch_env when running scylla-sstable as well. previously, these env variables are only applied when launching scylla as a daemon.

env = self._launch_env
except AttributeError:
env = os.environ
res = subprocess.run(common_args + sstables, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, check=True, env=env)
Copy link
Member

Choose a reason for hiding this comment

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

LGTM. This is almost identical to the change I made in my tree.

@fruch fruch merged commit 48fd2dc into scylladb:next Aug 29, 2023
3 checks passed
@tchaikov tchaikov deleted the scylla-node-env-1 branch August 29, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants