Add support Windows x64 MSVC openssl build #203
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@ddulesov, при всём уважении в таком виде патч не пригоден. Много где различия сводятся только к whitespace, и это его сильно раздувает. Но главное - выделение компилятора и опций компиляции не должно затрагивать исходники за исключением мест, которые не устраивают компилятор. |
|
ansi_terminal.h и ansi_terminal.c - GPL-3 (https://github.com/sol-prog/ansi-escape-codes-windows-posix-terminals-c-programming-examples/blob/master/LICENSE) |
Да, это обоснованное замечание. Лицензионное ограничение GPL v3 не позволяет включить его в исходном виде. В этом случае как поступить? |
По поводу форматирования.Проект не придерживается какого-то единого code guide. И это понятно, поскольку у разных модулей разные авторы. С этим как-то можно жить. Но есть и просто неряшливость, где в одной строке может быть и табуляция и пробелы. В этом патче множество мелких изменений приходится вносить в большое число файлов, в том числе и такие фрагменты. |
|
Приведение стиля явно должно делаться отдельным коммитом. Причёсывание тестов - тоже. Вы можете выделить изменения в CMakeLists, относящиеся к компиляции? Если да, то нужно сделать именно это. |
|
vt-alt
reviewed
Jan 28, 2020
| endif() | ||
|
|
||
| set(CMAKE_C_STANDARD 90) | ||
| set(CMAKE_C_STANDARD 11) |
There was a problem hiding this comment.
Зачем тут C11 если хватило бы C99?
There was a problem hiding this comment.
Да, в данном патче именно C11 не нужен. У меня он сохранился по причине использования атомарных операций в утилите gost1sum. В этом случае это must have.
Я бы в любом случае перевел сборку на C11. Этот стандарт поддерживается всеми современными компиляторами. В него внесены изменения, связанные с поддержкой выравнивания структур данных. Это позволит избавиться от compile/platform specific определений для указания выравнивания.
There was a problem hiding this comment.
Я тут ориентируюсь на openssl, потому что engine должен собираться там, где собирается openssl. Там скорее всего C90.
There was a problem hiding this comment.
Да, в данном патче именно C11 не нужен. У меня он сохранился по причине использования атомарных операций в утилите gost1sum. В этом случае это must have.
Я бы в любом случае перевел сборку на C11.
В Centos 6 gcc-4.4 без C11, а эта система до сих пор используется и поддерживается. Такие значительные изменения совместимости надо согласовывать, а лучше вообще не делать ближайшие 10 лет.
There was a problem hiding this comment.
Хотя gcc-4.х вышел до опубликования стандарта, он поддерживает его в draft варианте.
Но поскольку изменения не требуют явно новшеств стандарта , лучше сохранить в настройках С90.
@beldmit С первым утверждением согласен. Только сделать это нужно до прочих/моих изменений. |
|
Прямой взаимосвязи я не вижу. на ABI получаемого кода это не сказывается.
Это те же компиляторы. Хотя может быть вы знаете какие-то версии/платформы
, под которые openssl использует инструменты сборки, не поддерживающие
стандарт std c11?.
…On Tue, Jan 28, 2020 at 5:18 PM Dmitry Belyavskiy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In CMakeLists.txt
<#203 (comment)>:
> endif()
-set(CMAKE_C_STANDARD 90)
+set(CMAKE_C_STANDARD 11)
Я тут ориентируюсь на openssl, потому что engine должен собираться там,
где собирается openssl.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#203?email_source=notifications&email_token=AGQXOWRZC426QVFZ5XYTD7TRAA5DFA5CNFSM4KMQI6SKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTJYRPY#discussion_r371828077>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGQXOWWFEKSU3HFXZMUPYZLRAA5DFANCNFSM4KMQI6SA>
.
|
|
Мне тут подсказывают, что вы просто записали файл с заменой пробелов на табы. |
Да, в тех файлах, в которые вносились изменения и где была неопределенность с используемыми отступами |
beldmit
reviewed
Feb 2, 2020
|
|
||
| if(ASAN) | ||
| message(STATUS "address sanitizer enabled") | ||
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=address -g3 -fno-omit-frame-pointer") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
@ddulesov, можете выделить вот только этот кусок (и без повышения требований к версии CMake)? Кажется, задачу сборки MVCC он решает.
There was a problem hiding this comment.
не понимаю, что в этом куске нужно сделать?
ASAN - adress sanitizer - инструмент только для gcc и clang
There was a problem hiding this comment.
@beldmit На веб интерфейсе не понятно какой кусок имеется ввиду.
There was a problem hiding this comment.
Да, я уже понял. @ddulesov, я имел в виду вот до 40 строки, а не последний сегмент. Прошу прощения.
|
по поводу пробелов и табов это обычно знатные холивары бывают. можно отложить это "на потом". и приделать какой-нибудь clang-format или uncrustify в травис еще предполагаю, что будет холивар из-за переводов строки. который лечится в .gitattributes ничего из этого не стоит того, чтобы холиварить |
chipitsine
reviewed
Feb 4, 2020
| add_compile_options(/MP /WX /W4 /wd4100 /wd4267 /wd4706) | ||
| ENDif() | ||
|
|
||
| if(ASAN) |
There was a problem hiding this comment.
все хотел спросить. а в чем задумка "ASAN" ? плюс-минус так же можно пропихать -fsanitize в CMAKE_C_FLAGS
There was a problem hiding this comment.
Ещё отключаются perl и tcl тесты.
There was a problem hiding this comment.
а, про это тоже хотел спросить. с наивной точки зрения кажется, что тесты таки стоит гонять с включенными санитайзерами. если с выключенными, то есть причина ?
|
@chipitsine Тесты на Perl и Tcl вызывают бинарник При этом, тесты на C работают нормально. Это был короткий ответ. В принципе, на своей машине, зная где лежит asan runtime, я могу попробовать сделать так: Вроде бы всё ок, но: Я не готов разбираться со всякими false positive и memory leaks в perl-е. |
|
Заинсталил И что с этим делать? |
|
ну вот... никто не хочет чинить утечки памяти в перле :/ |
|
что в итоге с MSVC билдами? надо бы доделать эту штуку |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
rearange CMakeLists.txt control blocks
exclude some targets from Windows build
quility and secutity improvements:
getopt.h:
dedicate common code fragments from test_* to test.h file
fix issues with T(e), TE(e) macros
gostsum.c, gost12sum.c
gost_grasshopper_cipher.c
all affected source files with weird indents were reformated using four space indents