Closed Sashan closed 2 months ago
the changes look reasonable, but you have a fixup as your first commit, not sure that will autosquash properly
I've just force pushed repository with squashed changes.
bonus: it builds on MacOS too. Gave a try 1.1.1, 3.0 3.3 and 3.4-dev everything worked fine.
I did a bit of hacking, and a much bigger instruction comment in source/CMakeLists.txt. Mind of I simply push a fixup commit into this PR?
This got me to test the OpenSSLConfig.cmake we produce for using an OpenSSL build without installing it, and oh boy, I found a bug (fixed in openssl/openssl#24918)
I did a bit of hacking, and a much bigger instruction comment in source/CMakeLists.txt. Mind of I simply push a fixup commit into this PR?
please, go ahead and push your changes in.
Done. Don't forget to pull...
BTW, if you want to see what's actually happening when building:
$ cmake --build ./build --verbose
update CMakeList.txt so it can build evp_setpeer which has just landed to repository. I'm also doing a minor tweak to evp_setpeer.c to fix build on windows.
@levitte it would be nice to have those changes in so everyone can build tools on windows. thanks.
@levitte it would be nice to have those changes in so everyone can build tools on windows. thanks.
I agree. Might I encourage you to nudge others to review them?
ping @openssl/committers for second review
I pushed a fixup just now, changing CMakeLists.txt a little bit.
This moves all the dependencies on OpenSSL to the perf library, and also declares its public include directory.
With that, all executables only need to depend on the perf library, and the magic of CMake will take care of the rest.
please hold your new approvals/merging the cmake does not work on windows yet. this is the error I get when I try to build it
C:\Users\OpenSSL\work.openssl\perftools\source>cmake --build build-master-one
ninja: error: build.ninja:54: bad $-escape (literal $ must be written as $$)
INCLUDES = -IC:\Users\OpenSSL\work.openssl\perftools\source\$(PROJECT_...
^ near here
not sure if it is glitch in cmake or ninja on windows, this is triggered by addition of target_sources()
I'm still investigating the issue. will continue on Sunday evening.
The basename() is reworked so both separators ('/' and '\') are recognized on windows. Also function behavior got closer to behavior found on unix/linux
@Sashan Err, looks like you committed basename.o
Commits to be squashed when merging.
Commits to be squashed when merging.
I can squash commits and do a force push when I'll be doing s/posix/POSIX, would that be OK?
I can squash commits and do a force push when I'll be doing s/posix/POSIX, would that be OK?
I don't know about others, but when reviewing, I personally find it easier if commits are only added, with the squashing done on merge
Approved and labels updated
Merged. Thank you.