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

Use #if defined instead of #if and turn off -Werror for debug builds #68

Merged
merged 3 commits into from
Nov 29, 2023

Conversation

abhyshr
Copy link
Collaborator

@abhyshr abhyshr commented Nov 9, 2023

#if does not work correctly on Mac so switching to #if defined. Noticed this behavior for
#if (EXAGO_ENABLE_IPOPT || EXAGO_ENABLE_HIOP) so replaced it with
#if defined EXAGO_ENABLE_IPOPT || defined EXAGO_ENABLE_HIOP.

Also removed the -Werror flag for debug builds. sprintf is depecreated for clang and so -Werror flag results in zillions of errors. Note that the error is thrown not only for sprintf in ExaGO code, but also if it is any dependency library. Need to tackle this through a separate issue so turning off the -Werror for now.

@abhyshr abhyshr self-assigned this Nov 9, 2023
Copy link
Contributor

@cameronrutherford cameronrutherford left a comment

Choose a reason for hiding this comment

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

I was struggling to add the suggested changes in the GitHub IDE, but let's add the MacOS stuff into a conditional, and set everything else to the current setting.

@cameronrutherford
Copy link
Contributor

Also this is a +1 for #52

@cameronrutherford
Copy link
Contributor

I will PR into this with CMake suggestion. Still not sure why tests are failing on Newell + Ascent. Noteably two different tests are failing.

bjpalmer
bjpalmer previously approved these changes Nov 20, 2023
@cameronrutherford
Copy link
Contributor

Newell and Ascent are still failing so if someone can take a look into those failures then I am happy to merge

@cameronrutherford
Copy link
Contributor

Since this seems to have gotten lost in the comments - tests are failing on Newell and Ascent. Please fix

@cameronrutherford
Copy link
Contributor

Since this seems to have gotten lost in the comments - tests are failing on Newell and Ascent. Please fix

@abhyshr

@bjpalmer
Copy link
Collaborator

I just ran this branch on deception and tests 20 and 21 are passing. These were the ones that you were reporting as failing, correct? I'm only seeing failures for 17,18,19.

@cameronrutherford
Copy link
Contributor

I just ran this branch on deception and tests 20 and 21 are passing. These were the ones that you were reporting as failing, correct? I'm only seeing failures for 17,18,19.

Remember we still are skipping some tests in CI per #43 and #3, which @jaelynlitz is also helping resolve. I think 17,18,19 are all skipped, and so you should see Deception tests passing as is reflected in PR status.

You will only see test failures on Ascent and Newell, as is reflected in GitHub commit status.

abhyshr and others added 3 commits November 28, 2023 22:54
* Update CMakeLists.txt with macos specific compile flags

* Apply pre-commmit fixes

---------

Co-authored-by: cameronrutherford <[email protected]>
@cameronrutherford cameronrutherford enabled auto-merge (squash) November 29, 2023 05:04
@cameronrutherford cameronrutherford merged commit 8a9f7f7 into develop Nov 29, 2023
11 checks passed
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.

4 participants