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

solve portability issues for Ubuntu 16.04.4 LTS #298

Merged
merged 7 commits into from Mar 30, 2018

Conversation

Projects
None yet
4 participants
@garlick
Copy link
Member

garlick commented Mar 29, 2018

This PR contains the changes I needed in order to bulid flux-sched on an Ubuntu 16.04.4 LTS system using a side-installed version of boost-1.53. The problems solved are:

  • fix the build system so that configure --with-boost=path works
  • deal with new warnings with g++ version 5.4.0
  • avoid namespace collisions between boost and system (possibly also a g++ version issue, since this was not seen earlier)

I tested against the last version of flux-core known to work with flux-sched (two PR's ago, flux-framework/flux-core@cdc9e9b) and it works.

The only thing that's still not quite right is I needed to set LD_LIBRARY_PATH on the "make check" command line in order for in-tree test executables to find the boost libraries. I guess usually libtool magically takes care of this, but flux-sched doesn't use libtool.

Some of this was discussed in #297

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 29, 2018

Great cleanup across the board! Sorry about the collision issue due to using the using keyword. All changes look good to me.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 29, 2018

No prob! I will see if I can add a commit to skip the failing simulator testing, as discussed.

build: use AX_BOOST_GRAPH macro
Problem: --with-boost=path has no effect on the search
location for libboost_graph.so.

boost_graph is found with AC_CHECK_LIB, while the rest of boost
is found with purpose-build autoconf macros.  The macro for
boost_graph is included but not used.

Call AX_BOOST_GRAPH and adjust Makefiles accordingly.

@garlick garlick force-pushed the garlick:boost_cleanup branch from 0be7518 to d1310da Mar 30, 2018

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 30, 2018

Once this is rebased, this can go in.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 30, 2018

It's been rebased. Hopefully travis likes it.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 30, 2018

Uh oh, stuck here

  CXXLD    utilities/grug2dot
utilities_grug2dot-resource_gen_spec.o: In function `read_graphml<boost::adjacency_list<boost::vecS, boost::vecS, boost::directedS, Flux::resource_model::resource_pool_gen_t, Flux::resource_model::relation_gen_t> >':
/usr/include/boost/graph/graphml.hpp:214: undefined reference to `boost::read_graphml(std::istream&, boost::mutate_graph&, unsigned long)'
collect2: error: ld returned 1 exit status
@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 30, 2018

Uh oh, stuck here

Ah... I remember now. I think the reason I switched to AC_CHECK_LIB was AX_BOOST_GRAPH didn't add the boost graph library to the link line. Maybe we need to extend AX_BOOST_GRAPH further...

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 30, 2018

grug2dot successfully linked on a debian system with packaged boost 1.55, and on my desktop Ubuntu system with side-installed 1.53. Not sure why travis with packaged boost 1.54 cannot be linked or why that function is missing/not matched by this call.

I guess I'm assuming the link line is fine, and this has to do with the proposed namespace changes.

The prototype in /usr/include/boost/graph/graphml.hpp is

namespace boost {
...
void BOOST_GRAPH_DECL
read_graphml(std::istream& in, mutate_graph& g, size_t desired_idx);
...
} // namespace boost

I can't see any obvious namespace issues, though when I look at how boost::read_graphml() is called in our code, I find I need to review/learn more C++ to understand how the types manage to match, or what if anything the "using" change could do to impact that.

// resource_gen_spec.hpp
class resource_gen_spec_t {
public:
    resource_gen_spec_t ();
    resource_gen_spec_t (const resource_gen_spec_t &o);
    const gg_t &gen_graph ();
    const gen_meth_t to_gen_method_t (const std::string &s) const;
    int read_graphml (const std::string &ifn);
    int write_graphviz (const std::string &ofn, bool simple=false);

private:
    void setup_dynamic_property (boost::dynamic_properties &dp, gg_t &g);
    gg_t g;
    boost::dynamic_properties dp;
};

// resource_gen_spec.cpp
/*! Load resource generator recipe graph from a graphml file
 *
 *  \param ifn           resource generator recipe file in graphml
 *  \return              0 on success; -1 otherwise
 */
int resource_gen_spec_t::read_graphml (const string &ifn)
{
    int rc = 0;
    ifstream in_file (ifn.c_str ());
    if (!in_file.good ())
        return -1;

    try {
        boost::read_graphml (in_file, g, dp);
    } catch (boost::graph_exception &e) {
        cerr << e.what () << endl;
        rc = -1;
    }

    in_file.close ();
    return rc;
}
@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 30, 2018

Never mind, I tried dropping the namespace commit on a test branch, and that one fails in travis in the same place, so your lead is probably the right one @dongahn.

build: fix bug in AX_BOOST_GRAPH macro
Problem: AX_BOOST_GRAPH, in its search for the
graph library, matches libboost_graph_parallel.so
over libboost_graph.so, but graph_parallel is not
a replacement for graph.

Alter the glob used so that it doesn't match
graph_parallel.

@garlick garlick force-pushed the garlick:boost_cleanup branch from d1310da to 700cc31 Mar 30, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 30, 2018

The problem was AX_BOOST_GRAPH was picking up boost_graph_parallel instead of boost_graph.

Fixed the glob in there, and also tweaked warning options so that older compilers don't choke on the newer maybe-uninitialized warning option.

Forced a push, fingers crossed.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 30, 2018

Excellent!

boost_graph_parallel might later provide us w/ further scheduling parallelization. But for now, we want to stick w/ serial version!

@garlick garlick force-pushed the garlick:boost_cleanup branch 2 times, most recently from 05a2802 to 61e9b8e Mar 30, 2018

garlick added some commits Mar 29, 2018

build: [g++-5.4.0] suppress boost warnings
Problem: sched fails to compile with gcc-5.4.0
and boost-1.53 due to excessive compiler warnings
that are treated as errors.

Suppress the following warnings completely:
  unused-local-typedefs
  deprecated-declarations
  unused-variable
  maybe-uninitialized
build: add AX_BOOST_REGEX
Problem: boost_regex library, apparently a dependency of
boost_graph, cannot be found during linkage.

Add AX_BOOST_REGEX macro and adjust Makefile.am accordingly.
build: add BOOST_LDFLAGS
Problem: cannot find boost libraries when building test
executables.

When linking against a side installed boost, $(BOOST_LDFLAGS)
is required.  Add it.
resource: [cleanup] avoid 'using namespace boost'
Problem: with g++ 5.4.0, the integer types in stdint.h
(or cstdint) have the same names as types defined within
boost, resulting in "ambiguous type" errors.

Avoid 'using namespace boost' in Flux code.  Instead,
prefix all boost symbols with boost::
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 30, 2018

Coverage Status

Coverage remained the same at 68.968% when pulling 6856990 on garlick:boost_cleanup into 92fc364 on flux-framework:master.

build: add -Wno-error to resource build
Problem: clang-3.8 doesn't grok -Wno-maybe-uninitialized,
and gcc-5.4.0 doesn't grok -Wno-error=unknown-warning-option.

For now, add -Wno-error in the resource directory
and drop -Wno-maybe-uninitialized so we can get this built
on travis.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 30, 2018

Codecov Report

Merging #298 into master will not change coverage.
The diff coverage is 45.45%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #298   +/-   ##
=======================================
  Coverage   67.74%   67.74%           
=======================================
  Files          46       46           
  Lines        8692     8692           
=======================================
  Hits         5888     5888           
  Misses       2804     2804
Impacted Files Coverage Δ
resource/dfu_match_id_based.hpp 62% <ø> (ø) ⬆️
resource/resource_gen_spec.hpp 100% <ø> (ø) ⬆️
resource/resource_graph.hpp 59.25% <ø> (ø) ⬆️
resource/resource_gen.cpp 83.95% <100%> (ø) ⬆️
resource/resource_gen_spec.cpp 54.54% <33.33%> (ø) ⬆️
resource/utilities/resource-query.cpp 38.46% <33.33%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92fc364...6856990. Read the comment docs.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Mar 30, 2018

Finally passed travis by finding the right combination of warning options across all compilers,
dropping -Wno-maybe-uninitialized and adding -Wno-error (for resource directory only).

I left this "band-aid" as a separate commit so the original set of warning suppressions is there, and the rationale for the band-aid is too. Ideally we'll address the root cause of those "maybe-uninitialized" warnings and then be able to drop -Wno-error so new warnings don't creep in later.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 30, 2018

I left this "band-aid" as a separate commit so the original set of warning suppressions is there, and the rationale for the band-aid is too. Ideally we'll address the root cause of those "maybe-uninitialized" warnings and then be able to drop -Wno-error so new warnings don't creep in later.

Yes, we should look into this. Could you kindly open up an issue for this?

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Mar 30, 2018

OK. LGTM! Thank you @garlick.

@dongahn dongahn merged commit 027a522 into flux-framework:master Mar 30, 2018

3 of 4 checks passed

codecov/patch 45.45% of diff hit (target 67.74%)
Details
codecov/project 67.74% remains the same compared to 92fc364
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 68.968%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.