DAOS-1864 build: Components added: mchecksum and isa-l_crypt by ryon-jensen · Pull Request #26 · daos-stack/scons_local

ryon-jensen · 2019-07-15T15:02:33Z

Added isa-l_crypt component and changed mchecksum component to
always be available.

Signed-off-by: Ryon Jensen ryon.jensen@intel.com

Added isa-l_crypt component and changed mchecksum component to always be available. Signed-off-by: Ryon Jensen <ryon.jensen@intel.com>

daosbuild1

Style warning(s) for job https://build.hpdd.intel.com/job/daos-stack/job/scons_local/job/PR-26/1/
Please review https://wiki.hpdd.intel.com/display/DC/Coding+Rules

daosbuild1 · 2019-07-15T15:11:15Z

Test stage code_lint_check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/scons_local/view/change-requests/job/PR-26/1/execution/node/183/log

JohnMalmberg · 2019-07-15T15:35:53Z

+                          './configure --prefix=$ISAL_CRYPTO_PREFIX '
+                          '--libdir=$ISAL_CRYPTO_PREFIX/lib',
+                          'make $JOBS_OPT', 'make install'],
+                required_progs=['nasm', 'yasm'],


While nasm and yasm have overlaps in their abilities, depending on the product they can build it differently.
We should determine specifically which one is required.

The actual requirements is that isa-l looks for nasm >= 2.13 and if nasm fails that test it uses yasm.

This would be similar to what we do with stdatomic.h for openpa....just adding a configure check though a little more complex since it needs a version as well. Probably similar to what we do with go version

Ya, I just copied most of isal's definition for isal_crypto. I'll update both to specify the actual requirements better.

I haven't figured out how to conditionally specify which asm to use based on if yasm or nasm is installed and which versions, but then I started to wonder if the DAOS build should maintain that information or if it should just rely on the isal build to fail if dependencies aren't met?

With required_progs removed and nasm/yasm removed from system isal has a nice error message:

checking for yasm... no
no yasm
checking for nasm... no
no nasm
configure: error: No modern yasm or nasm found as required. Yasm should be 1.2.0 or later, and nasm should be v2.11.01 or later (v2.13 for AVX512).
BuildFailure: isal failed to build:
File "/home/rjensen1/workspace/daos/scons_local/SConstruct", line 53:
scons()
File "/home/rjensen1/workspace/daos/scons_local/SConstruct", line 41:
reqs.require(env, *env.get("REQUIRES"))
File "/home/rjensen1/workspace/daos/scons_local/prereq_tools/base.py", line 1141:
raise error

Better than what DAOS currently provides:

Checking whether nasm program exists...no
MissingSystemLibs: isal has unmet dependancies required for build:
File "/home/rjensen1/workspace/daos/scons_local/SConstruct", line 53:
scons()
File "/home/rjensen1/workspace/daos/scons_local/SConstruct", line 41:
reqs.require(env, *env.get("REQUIRES"))
File "/home/rjensen1/workspace/daos/scons_local/prereq_tools/base.py", line 1141:
raise error

Thoughts?

yeah you'd probably have to write a custom configure function. For now, relying on the underlying configure to tell you is fine IMO

we could probably fix that error message to at least provide the name of the dependency that is missing :-)

JohnMalmberg · 2019-07-15T15:37:55Z

-                              'make $JOBS_OPT', 'make install'],
-                    libs=['mchecksum'],
-                    out_of_src_build=True)
+    url = "https://github.com/mercury-hpc/mchecksum.git"


We don't currently use mchecksum on x86_64. Is something changing?

I thought we had removed mchecksum. Do we need it? Doesn't isal do the same things?

I don't think we actually need it, but I was including the component definition here so it was available for a tool I created that benchmarks the different libraries and checksums. I didn't think it would hurt anything having the component defined here if it isn't included in the required prereqs in the Sconscript for the daos libraries. But I can easily remove it if it makes things too convoluted.

If it's only used by your tool, you can add a define in your SConstruct (see daos SConstruct line 47-48 which define cmocka and readline). But there is probably no harm in having it here either.

jolivier23 · 2019-07-22T21:56:01Z

-                              'make $JOBS_OPT', 'make install'],
-                    libs=['mchecksum'],
-                    out_of_src_build=True)
+    url = "https://github.com/mercury-hpc/mchecksum.git"


I thought we had removed mchecksum. Do we need it? Doesn't isal do the same things?

jolivier23 · 2019-07-22T22:00:07Z

+                          './configure --prefix=$ISAL_CRYPTO_PREFIX '
+                          '--libdir=$ISAL_CRYPTO_PREFIX/lib',
+                          'make $JOBS_OPT', 'make install'],
+                required_progs=['nasm', 'yasm'],


This would be similar to what we do with stdatomic.h for openpa....just adding a configure check though a little more complex since it needs a version as well. Probably similar to what we do with go version

Updated patch

…to ryon-jensen/DAOS-1864

jolivier23 · 2019-07-29T20:34:01Z

                          'make install'], libs=['opa'],
                package='openpa-devel' if inst(reqs, 'openpa') else None)

-    if ARM_PLATFORM:


is this no longer needed @vvenkates27 ?

I'm including mchecksum on the DAOS side for a checksum timing util. I didn't think it was needed elsewhere, but I guess I'm not 100% sure. @vvenkates27 ??

mchecksum is not needed for ARM anymore as ISA-L started supporting ARM. I actually had a patch locally to remove the requirement for using mchecksum in the checksum library for ARM . So I think we can ignore my patch #22, as this patch seems to do the same as well.

vvenkates27 · 2019-08-01T20:22:55Z

I think this is overlapping with #22
I am not sure why we cannot merge that, Can some one help me get that pull request merged.. there is no reason why any of the builds should fail

vvenkates27 · 2019-08-03T21:15:03Z

I think this is overlapping with #22
I am not sure why we cannot merge that, Can some one help me get that pull request merged.. there is no reason why any of the builds should fail

I think we can ignore my patch as this is doing the same.

vvenkates27

I am ok with these changes, I think we need to merge another patch in DAOS I had to remove dependency of mchecksum in ARM for daos checksum library.

DAOS-1864 build: Components added: mchecksum and isa-l_crypt

508f6b6

Added isa-l_crypt component and changed mchecksum component to always be available. Signed-off-by: Ryon Jensen <ryon.jensen@intel.com>

ryon-jensen requested review from JohnMalmberg and jolivier23 July 15, 2019 15:02

daosbuild1 previously requested changes Jul 15, 2019

View reviewed changes

Comment thread components/__init__.py Outdated

Comment thread components/__init__.py Outdated

JohnMalmberg reviewed Jul 15, 2019

View reviewed changes

jolivier23 suggested changes Jul 22, 2019

View reviewed changes

review feedback

7284451

ryon-jensen requested a review from jolivier23 July 26, 2019 14:09

Merge branch 'master' of https://github.com/daos-stack/scons_local in…

7da55b7

…to ryon-jensen/DAOS-1864

ryon-jensen requested review from JohnMalmberg and daosbuild1 July 26, 2019 14:31

feedback

cf41af5

jolivier23 approved these changes Jul 29, 2019

View reviewed changes

jolivier23 requested a review from vvenkates27 July 29, 2019 20:34

vvenkates27 reviewed Aug 3, 2019

View reviewed changes

vvenkates27 approved these changes Aug 3, 2019

View reviewed changes

ryon-jensen requested a review from a team August 5, 2019 14:27

jolivier23 merged commit 56dfe0e into master Aug 6, 2019

jolivier23 deleted the ryon-jensen/DAOS-1864 branch August 6, 2019 23:14

Uh oh!

Conversation

ryon-jensen commented Jul 15, 2019

Uh oh!

daosbuild1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

daosbuild1 commented Jul 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvenkates27 commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvenkates27 commented Aug 3, 2019

Uh oh!

vvenkates27 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vvenkates27 commented Aug 1, 2019 •

edited

Loading