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 osx native build #374

Merged
merged 1 commit into from
Nov 14, 2020
Merged

Conversation

lumpfish
Copy link
Contributor

Signed-off-by: Simon Rushton srushton@redhat.com

Signed-off-by: Simon Rushton <srushton@redhat.com>
Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

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

LGTM (wait for releases to complete this week before merge).

IFLAGS=-I. -I$(HEADERDIR) -I$(JAVA_HOME)/include/$(shell uname | tr "[:upper:]" "[:lower:]") -I$(JAVA_HOME)/include -I/usr/include
CFLAGS=-D_JNI_IMPLEMENTATION_ -D_TRIVIAL_AGENT -O0 -g3 -pedantic -c -Wall -std=c99 -fPIC -fno-omit-frame-pointer -m64 -o $(OBJDIR)/$(SRC)$(OSUFFIX)
LFLAGS=-shared -m64 -o
CC=cc
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these indentation changes? The only real change in this PR seems to be the addition of -dynamiclib in LFLAGS. If we don't need the rest of the indentation changes, we probably should remove them from this PR for sake of consistency.

Copy link
Contributor Author

@lumpfish lumpfish Nov 10, 2020

Choose a reason for hiding this comment

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

Those "indentation changes" are replacing tabs which should only be used at the start of target commands with spaces - they should not have been there in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Just making sure it's a valid change.

Copy link
Contributor

@Mesbah-Alam Mesbah-Alam left a comment

Choose a reason for hiding this comment

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

LGTM

@Mesbah-Alam
Copy link
Contributor

Looks good to me. We should merge this PR and adoptium/aqa-tests#2048 after this week's releases are done.

@karianna karianna added this to the November 2020 milestone Nov 11, 2020
@smlambert smlambert merged commit 8e47f00 into adoptium:master Nov 14, 2020
@lumpfish lumpfish deleted the fix_osx_native_build branch November 26, 2020 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants