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

Running CMake should not leave unclean git trace #551

Merged
merged 2 commits into from
Aug 1, 2019

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Aug 1, 2019

Currently, running a build with CMake will overwrite the pre-existing header include/dmlc/build_config.h, leaving a dirty git trace. The reason for this behavior is described in #432 (review)

Unclean git trace is inconvenient. For example, let's say you are working with the XGBoost project contains dmlc-core as a Git submodule. At the root of the XGBoost directory, you run

git checkout feature_branch

to switch to feature_branch. This won't work, because the dmlc-core submodule had been modified by the last run of CMake. You'd then have to manually clean out dmlc-core by running

cd dmlc-core
git checkout -- include/dmlc/build_config.h
cd ..

Related: apache/mxnet#15575

Fix: CMake should create a brand new header that's not under version control. The header under version control is now called include/dmlc/build_config_default.h, and CMake will generate include/dmlc/build_config.h. The generated header include/dmlc/build_config.h is also registered in .gitignore, so running CMake will not leave dirty git trace.

Detail: CMakeLists.txt will define the symbol DMLC_CORE_USE_CMAKE to indicate that dmlc-core is being built with CMake, not GNU Make. This symbol won't appear when the user opts for GNU Make.

cc @larroy @apeforest

@hcho3
Copy link
Contributor Author

hcho3 commented Aug 1, 2019

@sriramch @rongou See if this would improve your development routine.

Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for this fix! Could you please also link the PR in MXNet that depends on this change?

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

LGTM.

@hcho3
Copy link
Contributor Author

hcho3 commented Aug 1, 2019

@apeforest After this is merged, I will file a pull request to MXNet to upgrade 3rdparty/dmlc-core submodule. No change in MXNet codebase will be necessary.

Copy link
Contributor

@larroy larroy left a comment

Choose a reason for hiding this comment

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

LGTM. Is this file not used when building with Make? Make doesn't need to be modified?

@hcho3
Copy link
Contributor Author

hcho3 commented Aug 1, 2019

@larroy GNU Make uses dmlc/build_config_default.h.

@sriramch
Copy link

sriramch commented Aug 1, 2019

@hcho3 thanks for informing. i haven't face this issue (yet) as i have been using gnu make thus far...

@hcho3 hcho3 merged commit 808f485 into dmlc:master Aug 1, 2019
@hcho3 hcho3 deleted the clean_build_config branch August 1, 2019 17:21
@rongou
Copy link
Contributor

rongou commented Aug 1, 2019

@hcho3 Thanks for fixing this! It's been so annoying. :)

Copy link

@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.

I face the unclean git trace issue multiple times. Thanks for the fix!

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.

7 participants