Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CI scripts #5

Closed
wants to merge 10 commits into from
Closed

Update CI scripts #5

wants to merge 10 commits into from

Conversation

NelsonVides
Copy link
Collaborator

No description provided.

@NelsonVides NelsonVides force-pushed the update_ci branch 7 times, most recently from 659ad0c to 9a08f31 Compare November 20, 2023 10:24
@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a8c90f2) 89.94% compared to head (c1845e7) 71.42%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main       #5       +/-   ##
===========================================
- Coverage   89.94%   71.42%   -18.52%     
===========================================
  Files           2        1        -1     
  Lines         179       14      -165     
  Branches       13        0       -13     
===========================================
- Hits          161       10      -151     
+ Misses         18        4       -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NelsonVides NelsonVides force-pushed the update_ci branch 9 times, most recently from 84f7100 to 8ff780f Compare November 20, 2023 12:54
@NelsonVides NelsonVides force-pushed the update_ci branch 2 times, most recently from ce9d4e9 to e54b4c3 Compare November 20, 2023 13:32
@NelsonVides
Copy link
Collaborator Author

@big-r81 hi there! Related to #4, I've been trying here to modernise CI and now also adding windows to it, so that we can know its build won't be broken by any future changes. However, I cannot get windows to build in this runner, it keeps complaining that it can't find the <openssl/sha.h> header files. Do you know how does your windows build manage to find this header? Perhaps you can help me here :)

@big-r81
Copy link
Contributor

big-r81 commented Nov 20, 2023

Hi, that's great to hear 🫶. We use vcpkg to install the OpenSSL 3 dev dependency. Afterwards we add the dirs to the env paths so that the compiler can use it.

rebar.config Outdated
"$CFLAGS -std=c99 -O0 -g -Wall -Wextra -fPIC --coverage"},
{"(linux|solaris|freebsd|netbsd|openbsd|dragonfly|darwin|gnu)",
"$LDLIBS -lcrypto --coverage"},
{"win32", "CFLAGS", "$CFLAGS ${OPENSSL_INSTALL_DIR}\include /O0 /DNDEBUG /Wall"},
Copy link
Contributor

Choose a reason for hiding this comment

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

@NelsonVides
this should work:

{"win32", "CFLAGS", "$CFLAGS /I${OPENSSL_INSTALL_DIR}/include /O2 /DNDEBUG /Wall"}

@big-r81
Copy link
Contributor

big-r81 commented Nov 20, 2023

or with special escaping:

{"win32", "CFLAGS", "$CFLAGS /I\"${OPENSSL_INSTALL_DIR}\\include\" /O2 /DNDEBUG /Wall"}

@NelsonVides NelsonVides force-pushed the update_ci branch 4 times, most recently from 472ecbb to 228d912 Compare November 20, 2023 17:26
@big-r81
Copy link
Contributor

big-r81 commented Nov 20, 2023

@NelsonVides Where do you set your env var OPENSSL_INSTALL_DIR?

@NelsonVides
Copy link
Collaborator Author

@big-r81
I've tried with several mechanisms but it keeps failing with

d:/a/fast_pbkdf2/fast_pbkdf2/c_src/fast_pbkdf2.c(35): fatal error C1083: Cannot open include file: 'openssl/sha.h': No such file or directory

I was expecting the env-vars to be set by any of the many gha plugins started here, or when installed by choco or vspkg, but none works. I haven't touched a windows machine in over 5 years so I honestly don't know much about how windows work here 🥲

@big-r81
Copy link
Contributor

big-r81 commented Nov 20, 2023

@NelsonVides
For example, if you install OpenSSL with choco you need to check where it is installed. Then you could set the path to the env var OPENSSL_INSTALL_DIR.

@big-r81
Copy link
Contributor

big-r81 commented Nov 20, 2023

@NelsonVides In #6 there is a version which works for the Windows runner. You can use this as an idea. 😉

@NelsonVides NelsonVides mentioned this pull request Nov 21, 2023
@NelsonVides
Copy link
Collaborator Author

@big-r81 Thanks for the PR! I cherry-picked your commit and rebased on top of the main branch, you're tagged there as a co-author in the commit :)
CI stopped running here, GHA didn't like my rebasing, so I created a new PR at #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants