Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

julia: migrate build process to cmake #16125

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

iblislin
Copy link
Member

@iblislin iblislin commented Sep 9, 2019

The original build process triggered by Pkg.build is
done by GNU make.

TODO

The original build process triggered by `Pkg.add` is
done by GNU make.
@iblislin iblislin added the Julia label Sep 9, 2019
@iblislin iblislin marked this pull request as ready for review October 26, 2019 08:36
@iblislin iblislin requested a review from szha as a code owner October 26, 2019 08:36
@@ -89,3 +90,21 @@ MARK_AS_ADVANCED(
OpenBLAS
)

# Check ILP64 data model for the case of Julia self-shipped `libopenblas64_.so`
Copy link
Member Author

Choose a reason for hiding this comment

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

Could anyone review this cmake script?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much, but maybe you can simplify a bit using CHECK_SOURCE_COMPILES and similar. https://gitlab.kitware.com/cmake/community/wikis/doc/tutorials/How-To-Write-Platform-Checks
Overall looks ok-ish to me.

Copy link
Member Author

@iblislin iblislin Dec 24, 2019

Choose a reason for hiding this comment

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

ah, looks greate, I will try

  • CHECK_SYMBOL_EXISTS

Copy link
Contributor

Choose a reason for hiding this comment

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

Any updates?

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry, I'm swamped... I will try it after 1/10.

@iblislin iblislin changed the title julia: migrate build process to cmake WIP: julia: migrate build process to cmake Dec 17, 2019
@iblislin iblislin changed the title WIP: julia: migrate build process to cmake julia: migrate build process to cmake Dec 21, 2019
@iblislin
Copy link
Member Author

I think this PR is ready for review.

@iblislin
Copy link
Member Author

@aaronmarkham This PR improves the integration of Julia's self-shipped libopenblas.
The Julia private OpenBLAS library exports all symbols with the 64_ suffix, and the modified cblas.h is provided as julia/deps/include/cblas.h.

I want to add a new build pipeline in the job ci/jenkins/mxnet-validation/unix-cpu via a follow up PR.
Is adding a new build pipeline fine for this case?

@aaronmarkham
Copy link
Contributor

@aaronmarkham This PR improves the integration of Julia's self-shipped libopenblas.
The Julia private OpenBLAS library exports all symbols with the 64_ suffix, and the modified cblas.h is provided as julia/deps/include/cblas.h.

I want to add a new build pipeline in the job ci/jenkins/mxnet-validation/unix-cpu via a follow up PR.
Is adding a new build pipeline fine for this case?

I'm not sure. Let me socialize this a bit and someone can get back to you, but maybe not until after the holiday...

Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

Have few suggestions/questions. Apologies if any of those are wrong/unintelligent. I don't quite understand cmake/julia but curious nonetheless.

julia/deps/build.jl Outdated Show resolved Hide resolved
# Otherwise we won't rebuild on an update.
run(`rm -f $_libdir/libmxnet.$(Libdl.dlext)`)
rm(_blddir, recursive=true, force=true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously it deleted the file libmxnet.* in the libdir
Now we are deleting the entire build directory. What changed with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, the final result is the same -- making a fresh build, not increamental build.

This build script is simliar to Python's setup.py.
It's for creating fresh build for user.

@@ -137,7 +138,7 @@ if !libmxnet_detected

run(download_cmd(package_url, "mxnet.7z"))
# this command will create the dir "usr\\lib"
run(`$exe7z e mxnet.7z *\\build\\* *\\lib\\* -y -ousr\\lib`)
run(`$exe7z e mxnet.7z "*\\build\\*" "*\\lib\\*" -y -ousr\\lib`) # TODO check it works on windows or not
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this imply we "might" break cmake build for windows? Can't we check this on windows before merging PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I checked it on Window, but forgot to remove the comment.
The previous code just poped some warning about quoting.

Co-Authored-By: Chaitanya Prakash Bapat <[email protected]>
julia/deps/build.jl Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants