A collection of small fixes #198

tiennou · 2017-07-11T21:07:53Z

This is a handful of small test-harness tweaks I collected while trying to have Xcode run the test cases (for the record, you need to set your PATH env-var correctly so Xcode finds docker, for each separate test case you need to debug).

The second commit moves our headers first in the list, so Xcode wouldn't try to use the ones from "system" locations (ie. Homebrew's /usr/local).

The last one hides OpenSSL's AES-CTR thingy inside its own initialization so it doesn't force other backends to NOP it.

mention-bot · 2017-07-11T21:07:55Z

@tiennou, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mback2k, @jas4711 and @vszakats to be potential reviewers.

tiennou · 2018-03-16T17:27:32Z

Rebased, anyone ?

willco007 · 2018-03-19T16:56:01Z

The AppVeyor builds are failing, could you look into why? Other than that, the actual refactoring looks good to me.

tiennou · 2018-03-19T19:19:18Z

Should be fixed. I fixup-ed some defines for missing headers on MSVC, and it's starting to come back green 🤞.

willco007 · 2018-03-20T17:55:22Z

Taking more of a look at this, some of the formatting needs to be updated/reverted to match the formatting guidelines. For example, no spaces after if/for and before ( and the else bracket formatting. That will cut down on the churn as well. After that's fixed, I'm ready to take these changes.

tiennou

Can do, I'm just confused, I haven't seen anything wrong per-se style-wise (apart from the things I commented on). Care to elaborate ?

From what I can see, the syntax is something like :

if/while(expr) {
    /* spaces */
}
else {
}

Right ?

tiennou · 2018-03-20T18:10:43Z

src/openssl.h

-  ENGINE_register_all_complete()
-#endif
-
+#define libssh2_crypto_init() _libssh2_openssl_crypto_init()


Needs a typedef.

tiennou · 2018-03-20T18:11:39Z

tests/openssh_fixture.c

-    else {
-        return rc;
-    }
+	return run_command(NULL, "docker stop %s", container_id);


tiennou · 2018-03-20T18:12:12Z

tests/session_fixture.c

+#endif
+	if(!wd) {
+#ifdef WIN32
+        char wd_buf[_MAX_PATH];


tiennou · 2018-03-20T18:12:31Z

tests/session_fixture.c

-        }
+    int rc;
+
+	setup_fixture_workdir();


willco007 · 2018-03-20T18:19:55Z

Oh that's my bad; I was diff'ing against an old branch accidentally so it was showing incorrect changes. Your changes looks good.

tiennou · 2018-03-20T18:23:29Z

Should be ready now, I took care of the issues I saw. Glad the style is OK 😉.

On my machine I have libssh2 1.6 installed, which was actually used instead of libssh2, so I got an error because of a missing LIBSSH2_ERROR_CHANNEL_WINDOW_FULL

* tests: Remove if-pyramids * tests: Switch run_command arguments * tests: Make run_command a vararg function * tests: Xcode doesn't obey CMake's test working directory * openssl: move manual AES-CTR cipher into crypto init * cmake: Move our include dir before all other include paths

Hopefully Xcode doesn't ignore this one. For sshd tests the runner script already changes current directory into the script's. Ref: 54bef4c libssh2#198 libssh2@10a5cbf

Hopefully Xcode doesn't break envvars. Added in 54bef4c libssh2#198 libssh2@10a5cbf

Before this patch libssh2 used a variety of solutions to pass the source directory to tests: `FIXTURE_WORKDIR` build-time macro (cmake), `FIXTURE_WORKDIR` envvar (unused), setting `srcdir` manually (autotools), setting current directory (cmake), and also `builddir` envvar (autotools) for passing current working dir to `mansyntax.sh`. This patch reduces this to using existing `srcdir` with autotools and setting it ourselves in CMake. This was mostly enabled by this recent patch: 4c9ed51 Details: - cmake: replace baked-in `FIXTURE_WORKDIR` macro with env. Added in 54bef4c #198 (2018-03-21) - rename `FIXTURE_WORKDIR` to `srcdir`, to match autotools. - cmake: add missing `srcdir` for algo and sshd tests. - session_fixture: stop `chdir()`-ing, rely on prefixing with `srcdir`. Changing current directory should be unnecessary after 4c9ed51 #801 (2023-02-24), that prefixes referenced input filenames with the `srcdir` envvar. The `srcdir` envvar was already exported by autotools, and now we're also setting it from CMake. - cmake: stop setting `WORKING_DIRECTORY`, rely on `srcdir` env. `WORKING_DIRECTORY` is no longer necessary, after passing `srcdir` to all tests, so they can find our source tree and keys/etc in it regardless of the current directory. Also this past commit hints that `WORKING_DIRECTORY` wasn't always working for this purpose as expected: "tests: Xcode doesn't obey CMake's test working directory" Ref: 10a5cbf - autotools: delete explicit `srcdir` for test env. Added in 13f8add (2015-07-02) automake documents `srcdir` as exported to the test environment: https://github.com/autotools-mirror/automake/blob/c04c4e8856e3c933239959ce18e16599fcc04a8b/doc/automake.texi#L9302-L9304 https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html It's mentioned in the docs back in 1997 and got a regression test in 2012. We can safely assume it to be available without setting it ourselves. - autotools: delete explicit `builddir`. Added in 13f8add (2015-07-02) It seems this wasn't necessary to make the above fix work, and `mansyntax.sh` is able to figure out the build workdir by reading `$PWD`. Our out-of-tree and `make distcheck` CI builds also work without it. Let us know if there is a scenario we're missing and needs this. Closes #1032

`MAXPATHLEN` is not present in some systems, e.g. GNU Hurd. Co-authored-by: Viktor Szakats Ref: 54bef4c libssh2#198 Fixes libssh2#1414 Closes libssh2#1415

Before this patch libssh2 used a variety of solutions to pass the source directory to tests: `FIXTURE_WORKDIR` build-time macro (cmake), `FIXTURE_WORKDIR` envvar (unused), setting `srcdir` manually (autotools), setting current directory (cmake), and also `builddir` envvar (autotools) for passing current working dir to `mansyntax.sh`. This patch reduces this to using existing `srcdir` with autotools and setting it ourselves in CMake. This was mostly enabled by this recent patch: 4c9ed51 Details: - cmake: replace baked-in `FIXTURE_WORKDIR` macro with env. Added in 54bef4c libssh2#198 (2018-03-21) - rename `FIXTURE_WORKDIR` to `srcdir`, to match autotools. - cmake: add missing `srcdir` for algo and sshd tests. - session_fixture: stop `chdir()`-ing, rely on prefixing with `srcdir`. Changing current directory should be unnecessary after 4c9ed51 libssh2#801 (2023-02-24), that prefixes referenced input filenames with the `srcdir` envvar. The `srcdir` envvar was already exported by autotools, and now we're also setting it from CMake. - cmake: stop setting `WORKING_DIRECTORY`, rely on `srcdir` env. `WORKING_DIRECTORY` is no longer necessary, after passing `srcdir` to all tests, so they can find our source tree and keys/etc in it regardless of the current directory. Also this past commit hints that `WORKING_DIRECTORY` wasn't always working for this purpose as expected: "tests: Xcode doesn't obey CMake's test working directory" Ref: libssh2@10a5cbf - autotools: delete explicit `srcdir` for test env. Added in 13f8add (2015-07-02) automake documents `srcdir` as exported to the test environment: https://github.com/autotools-mirror/automake/blob/c04c4e8856e3c933239959ce18e16599fcc04a8b/doc/automake.texi#L9302-L9304 https://www.gnu.org/software/automake/manual/html_node/Scripts_002dbased-Testsuites.html It's mentioned in the docs back in 1997 and got a regression test in 2012. We can safely assume it to be available without setting it ourselves. - autotools: delete explicit `builddir`. Added in 13f8add (2015-07-02) It seems this wasn't necessary to make the above fix work, and `mansyntax.sh` is able to figure out the build workdir by reading `$PWD`. Our out-of-tree and `make distcheck` CI builds also work without it. Let us know if there is a scenario we're missing and needs this. Closes libssh2#1032

`MAXPATHLEN` is not present in some systems, e.g. GNU Hurd. Co-authored-by: Viktor Szakats Ref: 54bef4c libssh2#198 Fixes libssh2#1414 Closes libssh2#1415

tiennou force-pushed the fixes branch from 13ab48f to da17309 Compare March 16, 2018 17:26

tiennou force-pushed the fixes branch from da17309 to 09e395f Compare March 17, 2018 00:15

tiennou mentioned this pull request Mar 17, 2018

Make hashing generic #240

Closed

willco007 mentioned this pull request Mar 18, 2018

Fix the EVP cipher meth memory leakage problem #241

Closed

tiennou force-pushed the fixes branch from 09e395f to 83f2dce Compare March 19, 2018 19:07

tiennou force-pushed the fixes branch 2 times, most recently from 14116a2 to c6f3d84 Compare March 19, 2018 21:10

tiennou commented Mar 20, 2018

View reviewed changes

tiennou added 5 commits March 20, 2018 19:22

tests: Remove if-pyramids

588782f

tests: Switch run_command arguments

db666d8

tests: Make run_command a vararg function

2395732

tests: Xcode doesn't obey CMake's test working directory

10a5cbf

openssl: move manual AES-CTR cipher into crypto init

f47a84c

tiennou force-pushed the fixes branch from c6f3d84 to f47a84c Compare March 20, 2018 18:22

cmake: Move our include dir before all other include paths

989b80a

On my machine I have libssh2 1.6 installed, which was actually used instead of libssh2, so I got an error because of a missing LIBSSH2_ERROR_CHANNEL_WINDOW_FULL

willco007 merged commit 54bef4c into libssh2:master Mar 20, 2018

tsengjun mentioned this pull request Mar 26, 2018

Fix the EVP cipher meth memory leakage problem #244

Merged

tiennou deleted the fixes branch April 4, 2018 22:12

vszakats added a commit to vszakats/libssh2 that referenced this pull request May 5, 2023

cmake: replace baked-in FIXTURE_WORKDIR macro with env

02b97a2

Hopefully Xcode doesn't break envvars. Added in 54bef4c libssh2#198 libssh2@10a5cbf

vszakats mentioned this pull request May 5, 2023

tests: simplify passing srcdir to tests #1032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

A collection of small fixes #198

A collection of small fixes #198

Uh oh!

tiennou commented Jul 11, 2017 •

edited

Loading

Uh oh!

mention-bot commented Jul 11, 2017

Uh oh!

tiennou commented Mar 16, 2018

Uh oh!

willco007 commented Mar 19, 2018

Uh oh!

tiennou commented Mar 19, 2018

Uh oh!

willco007 commented Mar 20, 2018

Uh oh!

tiennou left a comment

Uh oh!

tiennou Mar 20, 2018

Uh oh!

tiennou Mar 20, 2018

Uh oh!

tiennou Mar 20, 2018

Uh oh!

tiennou Mar 20, 2018

Uh oh!

willco007 commented Mar 20, 2018

Uh oh!

tiennou commented Mar 20, 2018

Uh oh!

Uh oh!

A collection of small fixes #198

A collection of small fixes #198

Uh oh!

Conversation

tiennou commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Jul 11, 2017

Uh oh!

tiennou commented Mar 16, 2018

Uh oh!

willco007 commented Mar 19, 2018

Uh oh!

tiennou commented Mar 19, 2018

Uh oh!

willco007 commented Mar 20, 2018

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

tiennou Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

tiennou Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

tiennou Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

tiennou Mar 20, 2018

Choose a reason for hiding this comment

Uh oh!

willco007 commented Mar 20, 2018

Uh oh!

tiennou commented Mar 20, 2018

Uh oh!

Uh oh!

tiennou commented Jul 11, 2017 •

edited

Loading