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

Fix up the CPP flags that are defined #1694

Merged
merged 1 commit into from
Jun 17, 2019
Merged

Fix up the CPP flags that are defined #1694

merged 1 commit into from
Jun 17, 2019

Conversation

ndmitchell
Copy link
Contributor

Required to load Shake with our repo, since that has platform-specific behaviour.

@ndmitchell ndmitchell requested a review from cocreature June 16, 2019 13:01
@ndmitchell ndmitchell requested a review from hurryabit as a code owner June 16, 2019 13:01
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

If it helps, I’m happy 👍

"-D" ++ TARGET_OS ++ "_HOST_OS",
"-D" ++ TARGET_ARCH ++ "_HOST_ARCH" ] -}
let target_defs =
-- NEIL: Patched to use System.Info instead of constants from CPP
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly broke the existing code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before I had commented out all these defines, when hacking over the CPP module in the first place. Shake has some #ifdef mingw32_HOST_OS and that wasn't defined, despite me being on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh, I missed the block comment around this. Now this makes a lot more sense :)

@neil-da neil-da merged commit 72f8e3e into master Jun 17, 2019
@neil-da neil-da deleted the fix-cpp-defines branch June 17, 2019 11:10
hsenag pushed a commit to hsenag/daml that referenced this pull request Sep 13, 2019
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