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

Fix make cookbook when certain cookbooks are guarded #3665

Merged
merged 1 commit into from Mar 2, 2017

Conversation

geektoni
Copy link
Contributor

This should fix issue #3520. These changes fix also make cookbook_sphinx_linkcheck.

The command make cookbook still gives me warnings, though, but I don't understand if they are caused by my changes or if they were already there.
See warnings here: http://pastebin.com/chxfGvaq

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

Thanks for the patch.

It is kind of dirty that we would have to blacklist an example twice don't you think?

I would prefer a solution where this only happens in a single place, i.e. the excluded cookbook pages are extracted from the excluded code listings ...

You can also pull out the whole functionality of blacklisting meta examples to a separate function that you then call from both the function that gives you all the excluded ones... I.e. make it more modular (current solution in develop is a hack anyways)

@geektoni
Copy link
Contributor Author

geektoni commented Mar 1, 2017

Mmmh, I think you are pretty right. It was more like a "quick fix" than a proper solution. I refactored the code a bit to make it more readable and modular. This new approach should be better.

<off_topic>
Can I ask why we have to deal with missing dependencies? We could simply add these libraries as required to compile shogun, instead of setting up these "workarounds". Obviously, I'm sure that you have a bunch of good reasons for doing so. ;)
</off_topic>

@karlnapf
Copy link
Member

karlnapf commented Mar 2, 2017

Yes, an overwhealming number of good reasons for that :)
Come to IRC and we can discuss

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

I like this much more now. Few changes and it is good to go

${CMAKE_SOURCE_DIR}/examples/meta/src/gaussian_processes/gaussian_process_regression.sg)
IF(NOT HAVE_NLOPT)
LIST(APPEND EXCLUDED_META_EXAMPLES
gaussian_processes/gaussian_process_regression.sg)
Copy link
Member

Choose a reason for hiding this comment

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

+1 this is cleaner!

FILE(GLOB_RECURSE META_EXAMPLE_LISTINGS ${CMAKE_SOURCE_DIR}/examples/meta/src/*.sg)
get_excluded_meta_examples()

# temporary hacks to exclude certain meta examples that have dependencies
Copy link
Member

Choose a reason for hiding this comment

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

I guess this comment can be removed now, as it is a bit nicer

SET(META_EXAMPLES ${META_EXAMPLE_LISTINGS} PARENT_SCOPE)
endfunction()

# Get the cookbook pages we want to exclude from build
function(find_excluded_meta_examples)
Copy link
Member

Choose a reason for hiding this comment

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

I think this function should be named differently, as it is about cookbook pages, not meta examples themselves

# This is made since Sphinx's exclude_patterns option wants
# the list's items separated by commas, but cmake's lists use
# semicolons instead.
# See: https://cmake.org/cmake/help/v3.3/command/list.html
Copy link
Member

Choose a reason for hiding this comment

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

good to put that comment

@@ -1,5 +1,7 @@
#PROJECT(sphinxdoc)
#cmake_minimum_required(VERSION 2.8.8)
include(FindMetaExamples)
find_excluded_meta_examples()
Copy link
Member

Choose a reason for hiding this comment

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

rename as said above

get_excluded_meta_examples()

FOREACH(META_EXAMPLE ${EXCLUDED_META_EXAMPLES})
LIST(APPEND EXCLUDED_COOKBOOK_PAGES examples/${META_EXAMPLE})
Copy link
Member

Choose a reason for hiding this comment

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

one thing to consider here: Some meta examples do not have cookbook pages. So they do not need to be excluded technically. Not sure whether that causes problems, but it is definitely cleaner to only work on the existing cookbook pages

@geektoni
Copy link
Contributor Author

geektoni commented Mar 2, 2017

I made the suggested changes @karlnapf.
By the way, I would definitely like to know more about these management choices about shogun dependencies. Are you shogun developers "always online" on IRC or there is a preferred time frame?

@karlnapf
Copy link
Member

karlnapf commented Mar 2, 2017

Fine to merge from my side. @vigsterkr ?

@karlnapf
Copy link
Member

karlnapf commented Mar 2, 2017

@geektoni IRC hours depend. I am UTC 11am-7pm usually. Others might differ.

@vigsterkr
Copy link
Member

go

@karlnapf
Copy link
Member

karlnapf commented Mar 2, 2017

Weird failures in travis, I just restarted to be safe. Realised one is from multithreaded octave and the other one was a timeout anyways....

@karlnapf karlnapf merged commit a185e00 into shogun-toolbox:develop Mar 2, 2017
@geektoni geektoni deleted the patch-9 branch March 2, 2017 19:08
karasikov pushed a commit to karasikov/shogun that referenced this pull request Apr 15, 2017
Fix `make cookbook` when certain cookbooks are guarded
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

3 participants