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

Avoid repeatedly enumerating submodules #8421

Merged
merged 2 commits into from
Nov 18, 2019
Merged

Conversation

niboshi
Copy link
Member

@niboshi niboshi commented Nov 12, 2019

Just a DRY refactoring of CMakeLists.txt.

@emcastillo emcastillo self-assigned this Nov 13, 2019
emcastillo
emcastillo previously approved these changes Nov 18, 2019
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

Might be better to confirm with someone who knows about windows?

@emcastillo emcastillo dismissed their stale review November 18, 2019 02:11

MSVC behavior unclear

target_link_options(chainerx PUBLIC /wholearchive:$<TARGET_FILE:chainerx_routines>)
target_link_options(chainerx PUBLIC /wholearchive:$<TARGET_FILE:chainerx_native>)
target_link_options(chainerx PUBLIC /wholearchive:$<TARGET_FILE:chainerx_testing>)
target_link_libraries(chainerx PRIVATE chainerx_routines)
Copy link
Member

Choose a reason for hiding this comment

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

Here chainerx_base was not linked, but on the new changeset, it is.
I am not sure if this might cause any kind of issue in windows.
I don't think it should at all but might be better to confirm it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in afacf69.
Unfortunately I don't have Windows environment to test it.
Although it's better to confirm the fix, we don't necessarily have to do it because Windows is currently not officially supported.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds perfectly fine to me.
Thanks for addressing it
LGTM then!

@emcastillo
Copy link
Member

Jenkins, test this please

@emcastillo emcastillo added cat:code-fix Code refactoring that does not change the behavior. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. labels Nov 18, 2019
@emcastillo emcastillo added this to the v7.0.0 milestone Nov 18, 2019
@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit afacf69, target branch master) succeeded!

@mergify mergify bot merged commit cbb1042 into chainer:master Nov 18, 2019
@niboshi niboshi deleted the cmake-dry branch November 18, 2019 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:code-fix Code refactoring that does not change the behavior. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants