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

[Gluten-825]Enable compiling in macOS. #827

Closed
wants to merge 1 commit into from

Conversation

Yohahaha
Copy link
Contributor

@Yohahaha Yohahaha commented Jan 10, 2023

What changes were proposed in this pull request?

Platform:
MacOS 12.3 (Intel)
Apple clang 13.1.6
Homebrew 3.6.17

This pr enabling compile Gluten in macOS without Docker, pr #819 do similar things with Docker.
Add some no-error compile options, and modify some code to get macOS compatibility.
#825

References:
CPU Affinity on macOS
GTest with CMake
-Wpessimizing-move

How was this patch tested?

manul test.

Contract me if any problem in your macOS.

Add some no-error compile options.
@Yohahaha
Copy link
Contributor Author

cc @CodingCat @PHILO-HE

@Yohahaha Yohahaha changed the title Enable compiling in macOS. [Gluten-825]Enable compiling in macOS. Jan 10, 2023
@github-actions
Copy link

#825

@FelixYBW
Copy link
Contributor

@CodingCat you may try this PR

@@ -908,18 +907,6 @@ Java_io_glutenproject_memory_alloc_NativeMemoryAllocator_bytesAllocated(JNIEnv*
JNI_METHOD_END(-1L)
}

JNIEXPORT void JNICALL Java_io_glutenproject_tpc_MallocUtils_mallocTrim(JNIEnv* env, jobject obj) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After search all project about this interface, I can not find any usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zhztheplayer Can we remove the API?

Copy link
Member

Choose a reason for hiding this comment

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

+1, it should be removed.

@CodingCat
Copy link
Contributor

CodingCat commented Jan 10, 2023

can we set a CI/CD step to build on mac? just to ensure that what is committed in this PR will not be broken

@CodingCat
Copy link
Contributor

also as discussed in slack, build_velox.sh also need some non-tiny changes....(is the correctness of the script covered by the current CI/CD?)

@CodingCat
Copy link
Contributor

I tested this PR, it didn't work in my local, seems GTEST changes is the deal breaker

@Yohahaha
Copy link
Contributor Author

After thinking about real usage, build and run in macOS without docker has less benefits, so i would close this pr.
#819 should be a better way.

@Yohahaha Yohahaha closed this Jan 10, 2023
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