Skip to content

TEST: add util/wrap.pl and use it#11110

Merged
openssl-machine merged 3 commits intoopenssl:masterfrom
levitte:add-util-opensslenv
Feb 27, 2020
Merged

TEST: add util/wrap.pl and use it#11110
openssl-machine merged 3 commits intoopenssl:masterfrom
levitte:add-util-opensslenv

Conversation

@levitte
Copy link
Copy Markdown
Member

@levitte levitte commented Feb 17, 2020 •
edited
Loading

util/wrap.pl is a script that defines the environment variables
OPENSSL_ENGINES and OPENSSL_MODULES, then calls the command line
that's given as its arguments.

On a POSIX platform, the command line call is done via
util/shlib_wrap.sh to ensure that the shared library paths are
correct. For other platforms, util/wrap.pl currently assumes that
similar things are already in place through other means.

This is an alternative to #11100

@levitte levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Feb 17, 2020
@levitte levitte requested a review from mspncp February 17, 2020 14:20
@levitte levitte mentioned this pull request Feb 17, 2020
Closed
@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 17, 2020

Looks promising. But I need to try it out first.

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 17, 2020

But I need to try it out first.

Of course! I assumed nothing less

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 17, 2020

I like it. How about giving it a cool and catchy name?

My favourite is just wrap.sh,

util/wrap.sh  apps/openssl  ...
util/wrap.sh  gdb --args test/drbgtest

but you could also call it env.sh if you prefer.

util/env.sh  apps/openssl  ...
util/env.sh  gdb --args test/drbgtest

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

I'm all for wrap.sh

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

It dawned on me that the functionality of the wrapping script is useful on VMS and Windows cmd as well, so I'm re-writing it in perl.

@levitte levitte changed the title TEST: add util/opensslenv.sh and use it TEST: add util/wrap.pl and use it Feb 18, 2020
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

Hold on a bit before approving, the narrative needs more extensive changes... which means that I need to make a squashing rebase so it doesn't get messier

@levitte levitte force-pushed the add-util-opensslenv branch from d56f06e to 6cee974 Compare February 18, 2020 07:56
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

Done. It'll be exciting to see if I got it right for Appveyor

@t8m
Copy link
Copy Markdown
Member

t8m commented Feb 18, 2020

There are merge conflicts already.

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

Argh...

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

Well, they were easy to deal with

@levitte levitte force-pushed the add-util-opensslenv branch from 6cee974 to 3d4e918 Compare February 18, 2020 10:13
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 18, 2020

Done

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 19, 2020

CI reset

@levitte levitte closed this Feb 19, 2020
@levitte levitte reopened this Feb 19, 2020
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 19, 2020

I hope to have solved the Appveyor failure. Travis failure was just a worker timeout

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 21, 2020

Fixed a syntax error in Test.pm and pushed it to your repo: 3a92c221cc16fa84b3ec913cdbe12ad3cdcf3afd.

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 21, 2020

Maybe you should handle the -h and --help options yourself, otherwise they get passed to the shlib_wrap.sh script

~/src/openssl$ util/wrap.pl -h

util/../util/shlib_wrap.sh: line 102: exec: -h: invalid option
exec: usage: exec [-cl] [-a name] [command [arguments ...]] [redirection ...]

respectively the exec command:

~/src/openssl$ util/wrap.pl --help

exec: exec [-cl] [-a name] [command [arguments ...]] [redirection ...]
    Replace the shell with the given command.
    
    Execute COMMAND, replacing this shell with the specified program.
    ARGUMENTS become the arguments to COMMAND.  If COMMAND is not specified,
    any redirections take effect in the current shell.
    
    Options:
      -a name	pass NAME as the zeroth argument to COMMAND
      -c	execute COMMAND with an empty environment
      -l	place a dash in the zeroth argument to COMMAND
    
    If the command cannot be executed, a non-interactive shell exits, unless
    the shell option `execfail' is set.
    
    Exit Status:
    Returns success unless COMMAND is not found or a redirection error occurs.

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 21, 2020

Works like a charm on Linux. I'll test it on Windows today or tomorrow.

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 21, 2020

Thanks.

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 23, 2020

Windows perl is... incorrect, it's exec seems to lose the exit code, so even the perl call perl -e "exec 'exit 1'" gives you the exit code 0...

So I had to hack wrap.pl to use system() instead. It should work out, I hope the theory holds

@levitte levitte force-pushed the add-util-opensslenv branch from 3ff2f40 to 1b0c798 Compare February 24, 2020 05:09
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 24, 2020

CIs are happy now!

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 24, 2020

Ping

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 24, 2020

Would you mind autosquashing the fixups (without moving the merge-base)? This would make it easier to check online what change goes where.

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 24, 2020

From a user viewpoint, all looks fine. I tested the script on windows and linux.

Side note: On my windows maschine the fipsinstall test crashes, but it seems to have nothing to do with the wrap.pl script. Since AppVeyor succeeds, this won't stop me from approving. I'll have to do a full rebuild and recheck the fipsinstall test. I'll open a separate issue if the problem persists.

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 24, 2020

I'll have to do a full rebuild and recheck the fipsinstall test.

@levitte the fact that you delete every file explicitly, seems to push Windows to its limits. nmake clean is choking on the overlong command lines...

NMAKE : fatal error U1095: Erweiterte Befehlszeile "del /Q /F apps\CA.pl apps\openssl.rc apps\tsget.pl crypto\aes\aes-x86_64.asm crypto\aes\aesni-mb-x86_64.asm crypto\aes\aesni-sha1-x86_64.asm crypto\aes\aesni-sha256-x86_64.asm crypto\aes\aesni-x86_64.asm crypto\aes\bsaes-x86_64.asm crypto\aes\vpaes-x86_64.asm crypto\bn\rsaz-avx2.asm crypto\bn\rsaz-x86_64.asm crypto\bn\x86_64-gf2m.asm crypto\bn\x86_64-mont.asm crypto\bn\x86_64-mont5.asm crypto\buildinf.h crypto\camellia\cmll-x86_64.asm crypto\chacha\chacha-x86_64.asm crypto\ec\ecp_nistz256-x86_64.asm crypto\ec\x25519-x86_64.asm crypto\md5\md5-x86_64.asm crypto\modes\aesni-gcm-x86_64.asm crypto\modes\ghash-x86_64.asm crypto\poly1305\poly1305-x86_64.asm crypto\rc4\rc4-md5-x86_64.asm crypto\rc4\rc4-x86_64.asm crypto\sha\keccak1600-x86_64.asm crypto\sha\sha1-mb-x86_64.asm crypto\sha\sha1-x86_64.asm crypto\sha\sha256-mb-x86_64.asm crypto\sha\sha256-x86_64.asm crypto\sha\sha512-x86_64.asm crypto\uplink-x86_64.asm crypto\whrlpool\wp-x86_64.asm crypto\x86_64cpuid.asm doc\man1\openssl-ca.pod doc\man1\openssl-cms.pod doc\man1\openssl-crl.pod doc\man1\openssl-dgst.pod doc\man1\openssl-dhparam.pod doc\man1\openssl-dsa.pod doc\man1\openssl-dsaparam.pod doc\man1\openssl-ec.pod doc\man1\openssl-ecparam.pod doc\man1\openssl-enc.pod doc\man1\openssl-gendsa.pod doc\man1\openssl-genpkey.pod doc\man1\openssl-genrsa.pod doc\man1\openssl-ocsp.pod doc\man1\openssl-passwd.pod doc\man1\openssl-pkcs12.pod doc\man1\openssl-pkcs7.pod doc\man1\openssl-pkcs8.pod doc\man1\openssl-pkey.pod doc\man1\openssl-pkeyparam.pod doc\man1\openssl-pkeyutl.pod doc\man1\openssl-rand.pod doc\man1\openssl-req.pod doc\man1\openssl-rsa.pod doc\man1\openssl-rsautl.pod doc\man1\openssl-s_client.pod doc\man1\openssl-s_server.pod doc\man1\openssl-s_time.pod doc\man1\openssl-smime.pod doc\man1\openssl-speed.pod doc\man1\openssl-spkac.pod doc\man1\openssl-srp.pod doc\man1\openssl-storeutl.pod doc\man1\openssl-ts.pod doc\man1\openssl-verify.pod doc\man1\openssl-x509.pod doc\man7\openssl_user_macros.pod engines\capi.def engines\dasync.def engines\e_padlock-x86_64.asm engines\ossltest.def engines\padlock.def libcrypto.def libcrypto.rc libssl.def libssl.rc providers\fips.def providers\legacy.def test\buildtest_aes.c test\buildtest_asn1.c test\buildtest_asn1t.c test\buildtest_async.c test\buildtest_bio.c test\buildtest_blowfish.c test\buildtest_bn.c test\buildtest_buffer.c test\buildtest_camellia.c test\buildtest_cast.c test\buildtest_cmac.c test\buildtest_cmp.c test\buildtest_cmp_util.c test\buildtest_cms.c test\buildtest_comp.c test\buildtest_conf.c test\buildtest_conf_api.c test\buildtest_configuration.c test\buildtest_core.c test\buildtest_core_names.c test\buildtest_core_numbers.c test\buildtest_crmf.c test\buildtest_crypto.c test\buildtest_ct.c test\buildtest_des.c test\buildtest_dh.c test\buildtest_dsa.c test\buildtest_dtls1.c test\buildtest_e_os2.c test\buildtest_ebcdic.c test\buildtest_ec.c test\buildtest_ecdh.c test\buildtest_ecdsa.c test\buildtest_engine.c test\buildtest_ess.c test\buildtest_evp.c test\buildtest_fips_names.c test\buildtest_hmac.c test\buildtest_http.c test\buildtest_idea.c test\buildtest_kdf.c test\buildtest_lhash.c test\buildtest_macros.c test\buildtest_md4.c test\buildtest_md5.c test\buildtest_mdc2.c test\buildtest_modes.c test\buildtest_obj_mac.c test\buildtest_objects.c test\buildtest_ocsp.c test\buildtest_opensslv.c test\buildtest_ossl_typ.c test\buildtest_params.c test\buildtest_pem.c test\buildtest_pem2.c test\buildtest_pkcs12.c test\buildtest_pkcs7.c test\buildtest_provider.c test\buildtest_rand.c test\buildtest_rand_drbg.c test\buildtest_rc2.c test\buildtest_rc4.c test\buildtest_ripemd.c test\buildtest_rsa.c test\buildtest_safestack.c test\buildtest_seed.c test\buildtest_self_test.c test\buildtest_serializer.c test\buildtest_sha.c test\buildtest_srp.c test\buildtest_srtp.c test\buildtest_ssl.c test\buildtest_ssl2.c test\buildtest_stack.c test\buildtest_store.c test\buildtest_symhacks.c test\buildtest_tls1.c test\buildtest_ts.c test\buildtest_txt_db.c test\buildtest_types.c test\buildtest_ui.c test\buildtest_whrlpool.c test\buildtest_x509.c test\buildtest_x509_vfy.c test\buildtest_x509v3.c test\p_test.def tools\c_rehash.pl" zu lang

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 24, 2020

@levitte the fact that you delete every file explicitly, seems to push Windows to its limits. nmake clean is choking on the overlong command lines...

Please create an issue

@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 24, 2020

the fipsinstall test crashes

Could that be related to #11150?

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 24, 2020

the fipsinstall test crashes

Could that be related to #11150?

It seems like my windows build directory was just in a desolate state. After cleaning all build artifacts manually and rebuilding, the fipsinstall test succeeded.

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 24, 2020

@levitte the fact that you delete every file explicitly, seems to push Windows to its limits. nmake clean is choking on the overlong command lines...

Please create an issue

Ok. After the testsuite has passed, I'll try to clean again and if the problem persists, I'll create an issue.

mspncp
mspncp approved these changes Feb 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mspncp mspncp left a comment

Choose a reason for hiding this comment

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

I have literally no idea what this VMS stuff is all about. It looks even more cryptic than all your Perl code I've seen up to now. ;-) But the rest looks reasonable and I tested it on Windows and Linux.

@mspncp mspncp added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 24, 2020
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 24, 2020

I'll introduce you to VMS some day 😉

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 24, 2020

Ok. After the testsuite has passed, I'll try to clean again and if the problem persists, I'll create an issue.

Done, see #11163.

@levitte levitte force-pushed the add-util-opensslenv branch from 1b0c798 to 2078938 Compare February 27, 2020 01:03
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 27, 2020

Had to rebase, INSTALL.md got updated with my changes in INSTALL

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 27, 2020 •
edited
Loading

Had to rebase, INSTALL.md got updated with my changes in INSTALL

What changes do you mean? Your branch before rebasing did not contain any changes in INSTALL, did it? So unless I'm missing something, it's reconfirmed.

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 27, 2020

Oh, forget it. I mixed up INSTALL and CHANGES in my mind ;-)

@mspncp
Copy link
Copy Markdown
Contributor

mspncp commented Feb 27, 2020

FWIW: I don't think this (trivial) rebase necessitates renewing the grace period.

@mspncp mspncp added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Feb 27, 2020
@levitte
Copy link
Copy Markdown
Member Author

levitte commented Feb 27, 2020

FWIW: I don't think this (trivial) rebase necessitates renewing the grace period.

I was hoping you'd say so, but wanted to leave that choice to you

@levitte
TEST: add util/wrap.pl and use it
285e299 
util/wrap.pl is a script that defines the environment variables
OPENSSL_ENGINES and OPENSSL_MODULES, then calls the command line
that's given as its arguments.

On a POSIX platform, the command line call is done via
util/shlib_wrap.sh to ensure that the shared library paths are
correct.  For other platforms, util/wrap.pl currently assumes that
similar things are already in place through other means.

Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from openssl#11110)
@levitte
Build file templates: don't set OPENSSL_{ENGINES,MODULES}
c3845ce 
Since we've now switched to use util/wrap.pl to wrap uninstalled
programs everywhere, there's no need to set the environment variables
OPENSSL_ENGINES and OPENSSL_MODULES globally for the tests.

Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from openssl#11110)
@levitte
Replace util/shlib_wrap.sh with util/wrap.pl in diverse docs
30a4cda 
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from openssl#11110)
@levitte levitte force-pushed the add-util-opensslenv branch from 2078938 to 30a4cda Compare February 27, 2020 07:51
@openssl-machine openssl-machine merged commit 30a4cda into openssl:master Feb 27, 2020
@levitte levitte deleted the add-util-opensslenv branch February 27, 2020 07:52
@t8m t8m mentioned this pull request Mar 5, 2020
Closed
romen
romen reviewed Mar 5, 2020
View reviewed changes
@t8m t8m mentioned this pull request Apr 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@romen romen romen left review comments

@mspncp mspncp mspncp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@levitte @mspncp @t8m @romen @openssl-machine