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

nmake compatibility #39

Merged
merged 13 commits into from
Jul 14, 2021
Merged

nmake compatibility #39

merged 13 commits into from
Jul 14, 2021

Conversation

egecetin
Copy link
Contributor

Changed Makefile to use the same file for make (Linux) and nmake (Windows)

Additionally gsw_check_functions.c array definitions changed because of Compiler Error C2131.

LIBRARY_SRCS = gsw_oceanographic_toolbox.c \
gsw_saar.c

# \
Copy link
Member

Choose a reason for hiding this comment

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

How does this block starting with line 10 work? Under what conditions is it active?

Copy link
Contributor Author

@egecetin egecetin Jul 13, 2021

Choose a reason for hiding this comment

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

nmake and make uses different syntax for ifdef, endif etc. these directives can raise a syntax error. Since nmake does not support multiline comments for nmake case line 11,12 and so on will be active lines but for make case they will be interpreted as comment block (except line 12).

By default nmake searches TOOLS.ini for definitions, by using this small trick we can include TOOLS.gcc file safely and can set all definitions for make case

Copy link
Contributor

Choose a reason for hiding this comment

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

While hacky looking, it seems that the comment behavior described is documented for both make and nmake.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I request just a minor change, and a little explanation, since I am far from being a "make" expert, and I don't know the Windows side at all. Overall, I'm happy to see this contribution.

gsw_check_functions.c Show resolved Hide resolved
@efiring efiring requested a review from ocefpaf July 13, 2021 18:34
@efiring
Copy link
Member

efiring commented Jul 13, 2021

@DocOtak, @ocefpaf: any comments?

@ocefpaf
Copy link
Member

ocefpaf commented Jul 13, 2021

@DocOtak, @ocefpaf: any comments?

I'm definetly not an expert on C and Windows. It would be nice to set a CI to test that compilation though. Let me see if I can make something for Linux that we can port to Windows later. (This is nor a blocker to merge this PR.)

@DocOtak
Copy link
Contributor

DocOtak commented Jul 13, 2021

I probably have a similar background re windows experience as @ocefpaf, I think we'll need to rely on contributions like this for windows support. I'd say let's get CI setup so we have some assurances that downstream projects won't break and we can merge.

@ocefpaf ocefpaf mentioned this pull request Jul 14, 2021
@ocefpaf
Copy link
Member

ocefpaf commented Jul 14, 2021

I'd say let's get CI setup so we have some assurances that downstream projects won't break and we can merge.

@egecetin I pushed the GitHub Action test to your branch and it is passing! Thanks!!

@egecetin
Copy link
Contributor Author

I'd say let's get CI setup so we have some assurances that downstream projects won't break and we can merge.

@egecetin I pushed the GitHub Action test to your branch and it is passing! Thanks!!

Thanks for your efforts in CI which makes this PR more meaningful. I think it can be merged now safely.

@efiring efiring merged commit 8531639 into TEOS-10:master Jul 14, 2021
@efiring
Copy link
Member

efiring commented Jul 14, 2021

Thank you!

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.

None yet

4 participants