You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/05/19 15:07:16 UTC
[GitHub] [incubator-mxnet] cbalint13 opened a new pull request #18365: Add optional MXNET_BUILD_SHARED
cbalint13 opened a new pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365
## Description ##
1. Add optional MXNET_BUILD_SHARED by default OFF.
2. With MXNET_BUILD_SHARED=ON no static version is build.
3. Also enable gcc 10.1.1 builds, bypass more -Werror checks.
## Checks ##
By default this PR should leave old build behaviour untouched.
## Comments ##
Building only the shared version is distro's releng favorite way.
Thank You !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu edited a comment on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632276816
@szha sure. In that case just set BUILD_SHARED_LIBS=0.
This is only about simplifying the cmake file to follow standard practice, reducing the amount of codes and hacks used by mxnet build system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631358262
Jenkins CI successfully triggered : [windows-cpu, edge, sanity, website, centos-gpu, clang, unix-cpu, miscellaneous, unix-gpu, centos-cpu, windows-gpu]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631362413
> > In current static mode a large static object and a small shared surrogate is build, that's it now.
>
> Actually not. There is a hack further down in the cmake file where the complete static object is included as is into the shared object. So there is not much difference between the two besides the format.
>
> > Can't make pure shared build without hurting CI very badly.
>
> What do you mean?
> > In current static mode a large static object and a small shared surrogate is build, that's it now.
>
> Actually not. There is a hack further down in the cmake file where the complete static object is included as is into the shared object. So there is not much difference between the two besides the format.
I am aware.
> > Can't make pure shared build without hurting CI very badly.
>
> What do you mean?
Will not pass CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 edited a comment on pull request #18365: Introduce MXNET_BUILD_STATIC=ON
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631102876
@leezu ,
Uhh, Ohh.
* Can't make pure shared build without hurting CI very badly.
* cmake's internal ```CMAKE_BUILD_SHARED``` would be perfect (but it is ```ON``` by default).
* I ended up using ```MXNET_BUILD_STATIC=ON``` to keep current behaviour, but also:
- ```CMAKE_BUILD_SHARED``` internal by default is ```ON``` (too big change for CI state).
- In current static mode a large static object and a small shared surrogate is build, that's it now.
- CI isn't hurt for now, by switching ```MXNET_BUILD_STATIC=OFF``` one can fix CI side.
- distro's releng can handle ```MXNET_BUILD_STATIC=OFF``` for now if they want on linux.
- When CI is solved we can replace ```MXNET_BUILD_STATIC``` with ```CMAKE_BUILD_SHARED``` internal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632278035
> @leezu static object should be useful in building standalone C++ inference programs. If the size of test is a problem, we can strip the binary after compilation.
- we did't get rid, still can build static objects if one desire
- in my opinion, too big for normal releng purposes (even stripped).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 edited a comment on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631362413
> > In current static mode a large static object and a small shared surrogate is build, that's it now.
>
> Actually not. There is a hack further down in the cmake file where the complete static object is included as is into the shared object. So there is not much difference between the two besides the format.
I am aware.
> > Can't make pure shared build without hurting CI very badly.
>
> What do you mean?
Will not pass CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Introduce MXNET_BUILD_STATIC=ON
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631123943
> In current static mode a large static object and a small shared surrogate is build, that's it now.
Actually not. There is a hack further down in the cmake file where the complete static object is included as is into the shared object. So there is not much difference between the two besides the format.
> Can't make pure shared build without hurting CI very badly.
What do you mean?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 edited a comment on pull request #18365: Introduce MXNET_BUILD_STATIC=ON
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631102876
@leezu ,
Uhh, Ohh.
* Can't make pure shared build without hurting CI very badly.
* cmake's internal ```CMAKE_BUILD_SHARED``` would be perfect (but it is ```ON``` by default).
* I ended with up using ```MXNET_BUILD_STATIC=ON``` to keep current behaviour:
- ```CMAKE_BUILD_SHARED``` internal by default is ```ON``` (too big change for CI state).
- In current static mode a large static object and a small shared surrogate is build, that's it now.
- CI isn't hurt for now, by switching ```MXNET_BUILD_STATIC=OFF``` one can fix CI side.
- distro's releng can handle ```MXNET_BUILD_STATIC=OFF``` for now if they want on linux.
- When CI is solved we can replace ```MXNET_BUILD_STATIC``` with ```CMAKE_BUILD_SHARED``` internal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Add optional MXNET_BUILD_SHARED
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-630906016
Thanks @cbalint13 . Actually we may improve the defaults for upcoming MXNet 2. I suggest to make the "build shared" the default and optionally allow to build only the static library. (ie following the CMAKE_BUILD_SHARED) approach.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631358153
@mxnet-bot run ci [all]
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] szha commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
szha commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632276050
@leezu static object should be useful in building standalone C++ inference programs. If the size of test is a problem, we can strip the binary after compilation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#discussion_r428813870
##########
File path: CMakeLists.txt
##########
@@ -663,29 +663,46 @@ if(UNIX)
# Create dummy file since we want an empty shared library before linking
set(DUMMY_SOURCE ${CMAKE_BINARY_DIR}/dummy.c)
file(WRITE ${DUMMY_SOURCE} "")
- list(APPEND MXNET_INSTALL_TARGETS mxnet_static)
- add_library(mxnet_static STATIC ${SOURCE})
- target_link_libraries(mxnet_static PUBLIC mshadow)
- target_compile_definitions(mxnet_static PUBLIC DMLC_LOG_FATAL_THROW=$<BOOL:${LOG_FATAL_THROW}>)
- add_library(mxnet SHARED ${DUMMY_SOURCE})
- target_link_libraries(mxnet PRIVATE ${BEGIN_WHOLE_ARCHIVE} $<TARGET_FILE:mxnet_static> ${END_WHOLE_ARCHIVE})
- target_link_libraries(mxnet PRIVATE mxnet_static)
- target_link_libraries(mxnet_static PUBLIC ${CMAKE_DL_LIBS})
- set_target_properties(mxnet_static PROPERTIES OUTPUT_NAME mxnet)
+ if(CMAKE_BUILD_SHARED)
+ add_library(mxnet SHARED ${SOURCE})
+ list(APPEND MXNET_INSTALL_TARGETS mxnet)
+ else()
+ add_library(mxnet_static STATIC ${SOURCE})
Review comment:
In particular we should get rid of the separate `mxnet_static` target
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Consider MXNET_BUILD_SHARED_LIBS flag
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632879807
Thanks @cbalint13
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu merged pull request #18365: Consider MXNET_BUILD_SHARED_LIBS flag
Posted by GitBox <gi...@apache.org>.
leezu merged pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632276816
@szha sure. In that case just set CMAKE_BUILD_SHARED=0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#discussion_r428813091
##########
File path: tests/CMakeLists.txt
##########
@@ -16,7 +16,7 @@ if(NOT GTEST_ROOT)
endif()
endif()
-if(NOT MSVC)
+if(NOT MSVC AND NOT CMAKE_BUILD_SHARED)
set(UNITTEST_STATIC_LINK ON)
endif()
Review comment:
These 3 lines can be deleted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#discussion_r428812794
##########
File path: CMakeLists.txt
##########
@@ -663,29 +663,46 @@ if(UNIX)
# Create dummy file since we want an empty shared library before linking
set(DUMMY_SOURCE ${CMAKE_BINARY_DIR}/dummy.c)
file(WRITE ${DUMMY_SOURCE} "")
- list(APPEND MXNET_INSTALL_TARGETS mxnet_static)
- add_library(mxnet_static STATIC ${SOURCE})
- target_link_libraries(mxnet_static PUBLIC mshadow)
- target_compile_definitions(mxnet_static PUBLIC DMLC_LOG_FATAL_THROW=$<BOOL:${LOG_FATAL_THROW}>)
- add_library(mxnet SHARED ${DUMMY_SOURCE})
- target_link_libraries(mxnet PRIVATE ${BEGIN_WHOLE_ARCHIVE} $<TARGET_FILE:mxnet_static> ${END_WHOLE_ARCHIVE})
- target_link_libraries(mxnet PRIVATE mxnet_static)
- target_link_libraries(mxnet_static PUBLIC ${CMAKE_DL_LIBS})
- set_target_properties(mxnet_static PROPERTIES OUTPUT_NAME mxnet)
+ if(CMAKE_BUILD_SHARED)
+ add_library(mxnet SHARED ${SOURCE})
Review comment:
You can remove `SHARED` here and delete the `else` clause below. cmake will use the correct library type based on `CMAKE_BUILD_SHARED`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Introduce MXNET_BUILD_STATIC=OFF
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631072533
Thanks @cbalint13. I wonder if we should only build the static library if `MXNET_BUILD_STATIC=ON`, instead of building both static and shared libraries. If yes, would `MXNET_BUILD_STATIC` even be needed or should we simply use the value of `CMAKE_BUILD_SHARED` (but default to `CMAKE_BUILD_SHARED=ON`). What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Introduce MXNET_BUILD_STATIC=ON
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631102876
@leezu ,
Uhh, Ohh.
* Can't make pure shared build without hurting CI very badly.
* cmake's internal ```CMAKE_BUILD_SHARED``` would be perfect (but it is ```ON``` by default).
* I ended with up using ```MXNET_BUILD_STATIC=ON``` due:
0. CMAKE_BUILD_SHARED internal by default is ```ON``` (too big change for CI state).
1. In current static mode a large static object and a small shared surrogate is build, that's it.
2. CI isn't hurt for now, by switching ```MXNET_BUILD_STATIC=OFF``` one can fix CI side.
3. distro's releng can handle ```MXNET_BUILD_STATIC=OFF``` for now if they want on linux.
4. When CI is solved we can replace MXNET_BUILD_STATIC with CMAKE_BUILD_SHARED internal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Consider BUILD_SHARED_LIBS flag
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632574190
> Adopting the standard `BUILD_SHARED_LIBS` may take additional work to make sure all dependencies + the CI logic correctly handle the result. It would however result into a more "distro-friendly" build you quote, as the dependencies will also be built dynamically.
Wow someone circumvent CI :1st_place_medal:
>
> But to keep things simple in this PR, it's fine to use an MXNet specific setting for now. I modified your PR slightly to reintroduce the MXNet specific option.
Thank you very much @leezu !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Consider BUILD_SHARED_LIBS flag
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632403523
Adopting the standard `BUILD_SHARED_LIBS` may take additional work to make sure all dependencies + the CI logic correctly handle the result. It would however result into a more "distro-friendly" build you quote, as the dependencies will also be built dynamically.
But to keep things simple in this PR, it's fine to use an MXNet specific setting for now. I modified your PR slightly to reintroduce the MXNet specific option.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Add optional MXNET_BUILD_SHARED
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-630953349
@leezu ,
Let's try, see how will pass CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631670657
> Will not pass CI.
Why do you think so. The CI does not rely on the `libmxnet.a`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632252702
> It seems, had the impression that all test units links libmxnet.a
That's part of the problem. It comes with huge link, disk space and CI network traffic overheads due to the number of unittests.
> `E AttributeError: dlsym(0x7fdb866c3810, MXGetLastError): symbol not found`
I'm not yet sure about this error. Could you address above comments first (or state that you disagree) and then let's look more closely into this issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 edited a comment on pull request #18365: Introduce MXNET_BUILD_STATIC=ON
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631102876
@leezu ,
Uhh, Ohh.
* Can't make pure shared build without hurting CI very badly.
* cmake's internal ```CMAKE_BUILD_SHARED``` would be perfect (but it is ```ON``` by default).
* I ended with up using ```MXNET_BUILD_STATIC=ON``` to keep current behaviour:
- CMAKE_BUILD_SHARED internal by default is ```ON``` (too big change for CI state).
- In current static mode a large static object and a small shared surrogate is build, that's it.
- CI isn't hurt for now, by switching ```MXNET_BUILD_STATIC=OFF``` one can fix CI side.
- distro's releng can handle ```MXNET_BUILD_STATIC=OFF``` for now if they want on linux.
- When CI is solved we can replace MXNET_BUILD_STATIC with CMAKE_BUILD_SHARED internal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Introduce MXNET_BUILD_STATIC=OFF
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-630994608
@leezu ,
UPDATE: Followed your proposal, updated the code.
* The way CI fails is beyond of this PR cause (noticed others fail too, i.e cuda.h not found).
* Is it still acceptable feature for trunk ?
Let me know if need more work on it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632010522
@leezu
> > Will not pass CI.
>
> Why do you think so. The CI does not rely on the `libmxnet.a`.
* It seems, had the impression that all test units links ```libmxnet.a```
Can advice me on MacOS CI failure ? Dont get the meaning of error (can't see what symbol):
```
E AttributeError: dlsym(0x7fdb866c3810, MXGetLastError): symbol not found
```
Thank You !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu edited a comment on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632276816
@szha sure. In that case just set CMAKE_BUILD_SHARED=0.
This is only about simplifying the cmake file to follow standard practice, reducing the amount of codes and hacks used by mxnet build system.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 edited a comment on pull request #18365: Introduce MXNET_BUILD_STATIC=ON
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-631102876
@leezu ,
Uhh, Ohh.
* Can't make pure shared build without hurting CI very badly.
* cmake's internal ```CMAKE_BUILD_SHARED``` would be perfect (but it is ```ON``` by default).
* I ended with up using ```MXNET_BUILD_STATIC=ON``` to keep current behaviour:
- CMAKE_BUILD_SHARED internal by default is ```ON``` (too big change for CI state).
- In current static mode a large static object and a small shared surrogate is build, that's it now.
- CI isn't hurt for now, by switching ```MXNET_BUILD_STATIC=OFF``` one can fix CI side.
- distro's releng can handle ```MXNET_BUILD_STATIC=OFF``` for now if they want on linux.
- When CI is solved we can replace ```MXNET_BUILD_STATIC``` with ```CMAKE_BUILD_SHARED``` internal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] leezu commented on a change in pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
leezu commented on a change in pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#discussion_r428813539
##########
File path: example/image-classification/predict-cpp/CMakeLists.txt
##########
@@ -5,11 +5,11 @@ OpenCV version >= 3 should be enabled and found to build image classification ex
return()
endif()
-if(NOT MSVC)
+if(NOT MSVC AND NOT CMAKE_BUILD_SHARED)
set(IMG_CLASSIFICATION_EXAMPLE_STATIC_LINK ON CACHE BOOL "\
Link mxnet library statically in the c++ image classification example")
else()
- # disable static linking on Windows
+ # disable static linking on Windows & CMAKE_BUILD_SHARED
set(IMG_CLASSIFICATION_EXAMPLE_STATIC_LINK OFF)
endif()
Review comment:
Let's delete this too and let cmake handle things automatically based on `CMAKE_BUILD_SHARED`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] mxnet-bot commented on pull request #18365: Add optional MXNET_BUILD_SHARED
Posted by GitBox <gi...@apache.org>.
mxnet-bot commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-630882923
Hey @cbalint13 , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:
- To trigger all jobs: @mxnet-bot run ci [all]
- To trigger specific jobs: @mxnet-bot run ci [job1, job2]
***
**CI supported jobs**: [edge, centos-cpu, unix-gpu, sanity, windows-gpu, miscellaneous, unix-cpu, website, clang, windows-cpu, centos-gpu]
***
_Note_:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 commented on pull request #18365: Consider CMAKE_BUILD_SHARED flag
Posted by GitBox <gi...@apache.org>.
cbalint13 commented on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-632297892
@leezu ,
* Proposed leap is big, someone need to adjust CI (out of my scope).
* I can re-propose PR it in earlier form (agree not elegant) and see that single one MacOS-CI issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-mxnet] cbalint13 edited a comment on pull request #18365: Add optional MXNET_BUILD_SHARED
Posted by GitBox <gi...@apache.org>.
cbalint13 edited a comment on pull request #18365:
URL: https://github.com/apache/incubator-mxnet/pull/18365#issuecomment-630953349
@leezu ,
Let's try, will see how will pass CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org