common: unify testconfig generation scripts

[janekmi]

This change is 

[grom72]

Reviewed all commit messages.
Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @janekmi)

a discussion (no related file):
I propose to use these as default values if not provided in the environment:

          OUTPUT_DIR: src/test
          PMEM_FS_DIR: /mnt/pmem0
          PMEM_FS_DIR_FORCE_PMEM: 0
          force_cacheline: 'True'
          force_byte: 'False'

With such a solution, the script can be easily used on the development machine to reproduce the CI environment.

---

---

utils/create-testconfig.sh line 28 at r1 (raw file):

    echo "OUTPUT_DIR is empty";
    exit 1;
fi

Let's assume some default values to simplify CI env reproduction.

Suggestion:

if [ -z "$OUTPUT_DIR" ]; then
    _OUTPUT_DIR=src/test
else
    _OUTPUT_DIR=$OUTPUT_DIR
fi

---

utils/create-testconfig.sh line 76 at r1 (raw file):

# generate Bash's test configuration file
cat << EOL > $OUTPUT_DIR/testconfig.sh

.

Suggestion:

cat << EOL > $_OUTPUT_DIR/testconfig.sh

---

utils/create-testconfig.sh line 89 at r1 (raw file):

# generate Python's test configuration file
cat << EOL > $OUTPUT_DIR/testconfig.py 

Suggestion:

cat << EOL > $_OUTPUT_DIR/testconfig.py

---

utils/docker/configure-tests.sh line 16 at r1 (raw file):

LONGDIR=LoremipsumdolorsitametconsecteturadipiscingelitVivamuslacinianibhattortordictumsollicitudinNullamvariusvestibulumligulaetegestaselitsemperidMaurisultriciesligulaeuipsumtinciduntluctusMorbimaximusvariusdolorid
# this path is ~3000 characters long
DIRSUFFIX="$LONGDIR/$LONGDIR/$LONGDIR/$LONGDIR/$LONGDIR"

DIRSUFFIX is used in unittest.sh

Code quote:

DIRSUFFIX

[osalyk]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 7 of 8 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @grom72)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

I propose to use these as default values if not provided in the environment:

          OUTPUT_DIR: src/test
          PMEM_FS_DIR: /mnt/pmem0
          PMEM_FS_DIR_FORCE_PMEM: 0
          force_cacheline: 'True'
          force_byte: 'False'

With such a solution, the script can be easily used on the development machine to reproduce the CI environment.

Do you think that

          force_cacheline: 'True'
          force_byte: 'False'

is more universally applicable compared to:

          force_cacheline: 'False'
          force_byte: 'True'

Note: The first one applies naturally to machines providing PMem at byte granularity whereas the second one to machines providing cache line granularity. The rule of thumb is that what is provided doesn't have to be forced.

---

---

utils/create-testconfig.sh line 28 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Let's assume some default values to simplify CI env reproduction.

Done.

---

utils/create-testconfig.sh line 76 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.

---

utils/create-testconfig.sh line 89 at r1 (raw file):

# generate Python's test configuration file
cat << EOL > $OUTPUT_DIR/testconfig.py 

Done.

---

utils/docker/configure-tests.sh line 16 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

DIRSUFFIX is used in unittest.sh

I see no value to this.

1. No user does such a thing.
2. Throughout the PMDK existence I have heard of no issue found thanks to this.
3. As far as I can tell it is very Windows-related: add long file path support  #2056

Please tell me if you still insist on having it back.

[codecov]

Codecov Report

Merging #5803 (c6f6e34) into master (2588011) will increase coverage by 2.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5803      +/-   ##
==========================================
+ Coverage   68.74%   70.98%   +2.24%     
==========================================
  Files         131      131              
  Lines       19677    19175     -502     
  Branches     3259     3193      -66     
==========================================
+ Hits        13527    13612      +85     
+ Misses       6150     5563     -587     

[grom72]

Reviewed 2 of 8 files at r1, 3 of 4 files at r2.
Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Do you think that

          force_cacheline: 'True'
          force_byte: 'False'

is more universally applicable compared to:

          force_cacheline: 'False'
          force_byte: 'True'

Note: The first one applies naturally to machines providing PMem at byte granularity whereas the second one to machines providing cache line granularity. The rule of thumb is that what is provided doesn't have to be forced.

No, I mean I want to use this script without any env variable to be set.
Let's select the one that is more useful -> cache line granularity

---

a discussion (no related file):
We should add DIRSUFFIX as mentioned in testconfig.sh.example otherwise, we create a shadow area with undefined(uninitialized) variables.

---

---

utils/create-testconfig.sh line 43 at r4 (raw file):

TEST_BUILD="debug nondebug"
build="['debug', 'nondebug']"
TEST_TIMEOUT=120m

This timeout is valid for PMem tests, for GHA 3 minutes was used.
I think that we should use anv variable in case of GHA to decrease it to 3 minutes only

Suggestion:

TEST_TIMEOUT=${TEST_TIMEOUT:-120m}

---

utils/docker/prepare-for-build.sh line 29 at r3 (raw file):

		force_cacheline=True \
		force_byte=True \
		../create-testconfig.sh

.

Suggestion:

	OUTPUT_DIR=$WORKDIR/src/test \
		PMEM_FS_DIR=/dev/shm \
		PMEM_FS_DIR_FORCE_PMEM=1 \
		force_cacheline=True \
		force_byte=True \
		TEST_TIMEOUT=3m \
		../create-testconfig.sh

---

utils/gha-runners/create-testconfig.sh line 31 at r4 (raw file):

cat >${CONF_PATH}/testconfig.py <<EOL
config = {
    'unittest_log_level': 1,

Why is it removed from the new file?

Code quote:

unittest_log_level

[grom72]

Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @janekmi)

---

utils/docker/configure-tests.sh line 16 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I see no value to this.

1. No user does such a thing.
2. Throughout the PMDK existence I have heard of no issue found thanks to this.
3. As far as I can tell it is very Windows-related: add long file path support  #2056

Please tell me if you still insist on having it back.

DIRSUFIX is used in the unittest.sh and shall be defined in testconfig.sh

[grom72]

Hmmm - something has been missing.

Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @janekmi)

[grom72]

https://github.com/pmem/pmdk/actions/runs/5586095077/jobs/10209997262

Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 1 of 1 files at r4, 1 of 1 files at r6.
Reviewable status: 7 of 8 files reviewed, 6 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

No, I mean I want to use this script without any env variable to be set.
Let's select the one that is more useful -> cache line granularity

Done.

---

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

We should add DIRSUFFIX as mentioned in testconfig.sh.example otherwise, we create a shadow area with undefined(uninitialized) variables.

There are no uninitialized variables in Bash. If it is not defined it is empty. DIRSUFFIX is completely optional. This fact is mentioned in testconfig.sh.example as well as obvious from the quick test framework browsing.

The bottom line is: I refuse to fulfil your wish. Please provide stronger arguments if you wish so. 😉

---

---

utils/create-testconfig.sh line 43 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

This timeout is valid for PMem tests, for GHA 3 minutes was used.
I think that we should use anv variable in case of GHA to decrease it to 3 minutes only

Yeah. It was. At least for Python tests. 🤦‍♂️
Done.

---

utils/docker/configure-tests.sh line 16 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

DIRSUFIX is used in the unittest.sh and shall be defined in testconfig.sh

As I said above. Yes, it is. But it is optional. I see no value to little value in using it.

---

utils/docker/prepare-for-build.sh line 29 at r3 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

.

Done.

---

utils/gha-runners/create-testconfig.sh line 31 at r4 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Why is it removed from the new file?

Done.

[janekmi]

Reviewed 1 of 2 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @grom72)

[grom72]

Reviewed 1 of 8 files at r1, 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

There are no uninitialized variables in Bash. If it is not defined it is empty. DIRSUFFIX is completely optional. This fact is mentioned in testconfig.sh.example as well as obvious from the quick test framework browsing.

The bottom line is: I refuse to fulfil your wish. Please provide stronger arguments if you wish so. 😉

Let's remove it.

---

[grom72]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

[janekmi]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

[grom72]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

[grom72]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

[osalyk]

Reviewed 3 of 4 files at r2, 1 of 2 files at r5, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @janekmi)