DAOS-15961 cart: Reorganize how envs are handled #14504

frostedcmos · 2024-06-04T03:28:57Z

Change to how cart deals with envariables:

env list is now controlled by CRT_ENV_LIST macro in crt_types_internal.h
all envs stored in structure generated from CRT_ENV_LST (g_env) now, ENV is read out at crt_env_init() time and env strings deallocated at crt_env_fini().
accsor functions/macros crt_env_init/fini/get/dump are provided.
string-type envs no longer need to be freed after retrieval.

With this change, any env to be used will need to appear on the list first, ensuring it gets dumped as well as ensuring proper name usage later. Env name typos when using crt_env_get() will now result in compile time errors

Required-githooks: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

Change to how cart deals with envariables: - env list is now controlled by CRT_ENV_LIST macro in crt_types_internal.h - all envs stored in structure generated from CRT_ENV_LST (g_env) now, ENV is read out at crt_env_init() time and env strings deallocated at crt_env_fini(). - accsor functions/macros crt_env_init/fini/get/dump are provided. - string-type envs no longer need to be freed after retrieval. With this change, any env to be used will need to appear on the list first, ensuring it gets dumped as well as ensuring proper name usage later. Env name typos when using crt_env_get() will now result in compile time errors Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

github-actions · 2024-06-04T03:29:19Z

Ticket title is 'Reorganize how cart handles envariables'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-15961

daosbuild1 · 2024-06-04T03:32:52Z

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/2/execution/node/384/log

daosbuild1 · 2024-06-04T03:37:27Z

Test stage Build RPM on EL 9 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/2/execution/node/315/log

daosbuild1 · 2024-06-04T03:38:32Z

Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/2/execution/node/308/log

daosbuild1 · 2024-06-04T03:38:33Z

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/2/execution/node/318/log

daosbuild1 · 2024-06-04T03:43:14Z

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/2/execution/node/358/log

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 · 2024-06-04T05:52:24Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14504/3/testReport/

daosbuild1 · 2024-06-04T11:42:37Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/3/execution/node/1188/log

- add flag to indicate whether g_envs was inited - assert on crt_env_get() if called when env is not inited - move crt_env_get() in utils after crt_init() Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 · 2024-06-04T19:35:37Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14504/4/testReport/

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 · 2024-06-04T21:36:51Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14504/5/testReport/

daosbuild1 · 2024-06-05T07:10:54Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/6/execution/node/1184/log

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 · 2024-06-05T22:43:37Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/9/execution/node/1229/log

setting and querying env. Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

daosbuild1 · 2024-06-06T00:02:01Z

Test stage Build on Leap 15.5 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-14504/11/execution/node/502/log

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

soumagne

a few minor things but looks good to me otherwise.

soumagne · 2024-06-07T23:13:06Z

src/cart/crt_init.c

@@ -13,48 +13,11 @@
 #include "crt_internal.h"

 struct crt_gdata	crt_gdata;
+struct crt_envs_t       g_envs;


we should be consistent and prefix global variables with crt_, even more if it's not static.

soumagne · 2024-06-07T23:14:32Z

src/cart/crt_init.c

@@ -90,6 +53,9 @@ crt_lib_init(void)
 	crt_gdata.cg_rpcid = start_rpcid;
 	crt_gdata.cg_num_cores = sysconf(_SC_NPROCESSORS_ONLN);
 	crt_gdata.cg_iv_inline_limit = 19456; /* 19KB */
+
+	/* envs not inited until crt_init() time */
+	memset(&g_envs, 0x0, sizeof(struct crt_envs_t));


you could do sizeof(g_envs) to not worry about the type.

soumagne · 2024-06-07T23:16:45Z

src/cart/crt_internal_types.h

+
+#define ENV_STR_NO_PRINT(x) ENV_STR(x)
+
+struct crt_envs_t {


we should not not use _t after struct names :)

soumagne · 2024-06-07T23:21:27Z

src/cart/crt_internal_types.h

+#undef ENV_STR
+#undef ENV_STR_NO_PRINT
+
+extern struct crt_envs_t g_envs;


ideally you'd want to use __attribute__ ((visibility ("hidden"))) for all global private declarations so that they remain hidden from public users.

hmm.. i remember before we had something in scons to make everything hidden by default, but i cant find it. Does it ring any bells @jolivier23 ?

anyway, i tried adding visibility hidden here and it has issues linking numerous files:
build/release/gcc/src/cart/utils/s_crt_utils.os: In function write_completion_file':
/home/aaoganez/github/daos_master/src/cart/utils/crt_utils.c:104: undefined reference to crt_genvs' /home/aaoganez/github/daos_master/src/cart/utils/crt_utils.c:104: undefined reference to crt_genvs'
/home/aaoganez/github/daos_master/src/cart/utils/crt_utils.c:104: undefined reference to crt_genvs'

Ah ok well if you use it in crt_utils then it's not really private 😄

Yeah, we did this in CPPR and IOF via an option to the linker and then defined a PUBLIC define to mark things visible

Looks like it carried over to src/client/dfuse/SConstruct where we set -fvisibility=hidden and then we use DFUSE_PUBLIC to mark the visible routines.

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Change to how cart deals with envariables: - env list is now controlled by CRT_ENV_LIST macro in crt_types_internal.h - all envs stored in structure generated from CRT_ENV_LST (crrt_genv) now, ENV is read out at crt_env_init() time and env strings deallocated at crt_env_fini(). - accsor functions/macros crt_env_init/fini/get/dump are provided. - string-type envs no longer need to be freed after retrieval. - Change cart utility function to populate crt_init_options_t instead of setting and querying env. With this change, any env to be used will need to appear on the list first, ensuring it gets dumped as well as ensuring proper name usage later. Env name typos when using crt_env_get() will now result in compile time errors Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Change to how cart deals with envariables: - env list is now controlled by CRT_ENV_LIST macro in crt_types_internal.h - all envs stored in structure generated from CRT_ENV_LST (crrt_genv) now, ENV is read out at crt_env_init() time and env strings deallocated at crt_env_fini(). - accsor functions/macros crt_env_init/fini/get/dump are provided. - string-type envs no longer need to be freed after retrieval. - Change cart utility function to populate crt_init_options_t instead of setting and querying env. With this change, any env to be used will need to appear on the list first, ensuring it gets dumped as well as ensuring proper name usage later. Env name typos when using crt_env_get() will now result in compile time errors Signed-off-by: Alexander A Oganezov <[email protected]>

frostedcmos added 2 commits June 4, 2024 03:25

style

096319a

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

- fix build errors, memset g_envs to 0

304b8bd

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

jolivier23 previously approved these changes Jun 4, 2024

View reviewed changes

frostedcmos dismissed jolivier23’s stale review via 5499d54 June 4, 2024 18:55

formatting

1f8a706

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Merge branch 'master' into aaoganez/DAOS-15961

e88c8ab

frostedcmos marked this pull request as ready for review June 5, 2024 00:22

frostedcmos requested review from a team as code owners June 5, 2024 00:22

frostedcmos requested review from jolivier23, soumagne, jgmoore-or and mchaarawi June 5, 2024 00:22

frostedcmos added 3 commits June 5, 2024 08:47

- remove wrong d_freenv, causing segfaults in ci testing

6d3563d

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

Merge branch 'master' into aaoganez/DAOS-15961

458dbbf

Merge branch 'master' into aaoganez/DAOS-15961

c4a184e

frostedcmos added 2 commits June 5, 2024 23:46

- Change cart utility function to populate crt_init_options_t instead of

96e6794

setting and querying env. Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

style

de5074f

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

target-c warning fix

e32ffbb

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

jolivier23 previously approved these changes Jun 7, 2024

View reviewed changes

jgmoore-or previously approved these changes Jun 7, 2024

View reviewed changes

soumagne requested changes Jun 7, 2024

View reviewed changes

frostedcmos added 2 commits June 8, 2024 00:42

Merge branch 'master' into aaoganez/DAOS-15961

2b449c8

- address review feedback

9e11079

Required-githooks: true Signed-off-by: Alexander A Oganezov <[email protected]>

frostedcmos dismissed stale reviews from jgmoore-or and jolivier23 via 9e11079 June 8, 2024 00:58

frostedcmos requested review from soumagne, jgmoore-or and jolivier23 June 8, 2024 00:59

soumagne approved these changes Jun 8, 2024

View reviewed changes

jolivier23 approved these changes Jun 11, 2024

View reviewed changes

frostedcmos merged commit 993558e into master Jun 11, 2024
52 checks passed

frostedcmos deleted the aaoganez/DAOS-15961 branch June 11, 2024 17:11

cdavis28 mentioned this pull request Aug 12, 2024

Merge upstream/release/2.6 into upstream/google/2.6 #14916

Merged

mjmac mentioned this pull request Nov 13, 2024

mjmac/DAOS 16787 google 2.6 #15498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-15961 cart: Reorganize how envs are handled #14504

DAOS-15961 cart: Reorganize how envs are handled #14504

frostedcmos commented Jun 4, 2024

github-actions bot commented Jun 4, 2024 •

edited

Loading

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 5, 2024

daosbuild1 commented Jun 5, 2024

daosbuild1 commented Jun 6, 2024

soumagne left a comment

soumagne Jun 7, 2024

frostedcmos Jun 8, 2024

soumagne Jun 7, 2024

frostedcmos Jun 8, 2024

soumagne Jun 7, 2024

frostedcmos Jun 8, 2024

soumagne Jun 7, 2024

frostedcmos Jun 8, 2024

frostedcmos Jun 8, 2024

soumagne Jun 8, 2024

jolivier23 Jun 11, 2024

jolivier23 Jun 11, 2024

DAOS-15961 cart: Reorganize how envs are handled #14504

DAOS-15961 cart: Reorganize how envs are handled #14504

Conversation

frostedcmos commented Jun 4, 2024

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jun 4, 2024 • edited Loading

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 4, 2024

daosbuild1 commented Jun 5, 2024

daosbuild1 commented Jun 5, 2024

daosbuild1 commented Jun 6, 2024

soumagne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 4, 2024 •

edited

Loading