solve portability issues for Ubuntu 16.04.4 LTS #298

garlick · 2018-03-29T22:23:50Z

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 · 2018-03-29T22:46:08Z

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

garlick · 2018-03-29T22:54:59Z

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

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.

dongahn · 2018-03-30T00:36:53Z

Once this is rebased, this can go in.

garlick · 2018-03-30T00:40:30Z

It's been rebased. Hopefully travis likes it.

garlick · 2018-03-30T00:58:21Z

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 · 2018-03-30T01:02:45Z

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 · 2018-03-30T14:42:57Z

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 · 2018-03-30T15:39:02Z

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.

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 · 2018-03-30T21:20:14Z

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 · 2018-03-30T21:30:48Z

Excellent!

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

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

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.

Problem: cannot find boost libraries when building test executables. When linking against a side installed boost, $(BOOST_LDFLAGS) is required. Add it.

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 · 2018-03-30T22:07:54Z

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

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 · 2018-03-30T22:31:59Z

Codecov Report

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

@@           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 · 2018-03-30T22:56:40Z

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 · 2018-03-30T22:58:32Z

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 · 2018-03-30T23:00:51Z

OK. LGTM! Thank you @garlick.

garlick mentioned this pull request Mar 29, 2018

jsc failures after flux-core changes #299

Closed

garlick force-pushed the boost_cleanup branch from 0be7518 to d1310da Compare March 30, 2018 00:36

garlick mentioned this pull request Mar 30, 2018

build: configure should detect unsupported boost versions #297

Closed

build: fix bug in AX_BOOST_GRAPH macro

d8f3f2d

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 force-pushed the boost_cleanup branch from d1310da to 700cc31 Compare March 30, 2018 21:17

garlick force-pushed the boost_cleanup branch 2 times, most recently from 05a2802 to 61e9b8e Compare March 30, 2018 21:46

garlick added 4 commits March 30, 2018 14:48

build: add AX_BOOST_REGEX

cb1d0f9

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

db52816

Problem: cannot find boost libraries when building test executables. When linking against a side installed boost, $(BOOST_LDFLAGS) is required. Add it.

build: add -Wno-error to resource build

6856990

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.

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

garlick mentioned this pull request Mar 30, 2018

address warnings in resource/resource_gen.cpp etc, then revert workaround #300

Closed

grondo mentioned this pull request May 11, 2018

Need 0.5.0 Release #340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solve portability issues for Ubuntu 16.04.4 LTS #298

solve portability issues for Ubuntu 16.04.4 LTS #298

garlick commented Mar 29, 2018

dongahn commented Mar 29, 2018

garlick commented Mar 29, 2018

dongahn commented Mar 30, 2018

garlick commented Mar 30, 2018

garlick commented Mar 30, 2018

dongahn commented Mar 30, 2018

garlick commented Mar 30, 2018 •

edited

garlick commented Mar 30, 2018

garlick commented Mar 30, 2018

dongahn commented Mar 30, 2018

coveralls commented Mar 30, 2018 •

edited

codecov-io commented Mar 30, 2018 •

edited

garlick commented Mar 30, 2018

dongahn commented Mar 30, 2018

dongahn commented Mar 30, 2018

solve portability issues for Ubuntu 16.04.4 LTS #298

solve portability issues for Ubuntu 16.04.4 LTS #298

Conversation

garlick commented Mar 29, 2018

dongahn commented Mar 29, 2018

garlick commented Mar 29, 2018

dongahn commented Mar 30, 2018

garlick commented Mar 30, 2018

garlick commented Mar 30, 2018

dongahn commented Mar 30, 2018

garlick commented Mar 30, 2018 • edited

garlick commented Mar 30, 2018

garlick commented Mar 30, 2018

dongahn commented Mar 30, 2018

coveralls commented Mar 30, 2018 • edited

codecov-io commented Mar 30, 2018 • edited

Codecov Report

garlick commented Mar 30, 2018

dongahn commented Mar 30, 2018

dongahn commented Mar 30, 2018

garlick commented Mar 30, 2018 •

edited

coveralls commented Mar 30, 2018 •

edited

codecov-io commented Mar 30, 2018 •

edited