Skip to content

A few small fixes for cleaner codebase and compilations#466

Merged
aquette merged 4 commits intonetworkupstools:masterfrom
jimklimov:smallfix
Aug 28, 2017
Merged

A few small fixes for cleaner codebase and compilations#466
aquette merged 4 commits intonetworkupstools:masterfrom
jimklimov:smallfix

Conversation

@jimklimov
Copy link
Copy Markdown
Member

No description provided.

@aquette aquette merged commit 51cfa6d into networkupstools:master Aug 28, 2017
Copy link
Copy Markdown
Member

@clepple clepple left a comment

Choose a reason for hiding this comment

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

I am concerned that the CFLAGS (should be CXXFLAGS, but that's beside the point) are being hardcoded here. What is the reasoning?

Copy link
Copy Markdown
Member

@clepple clepple left a comment

Choose a reason for hiding this comment

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

Although dstate.h is not exported, note that timehead.h is not all that useful anymore: #332

@jimklimov
Copy link
Copy Markdown
Member Author

I am concerned that the CFLAGS (should be CXXFLAGS, but that's beside the point) are being hardcoded here. What is the reasoning?

Building the full distcheck suite on an older debian release, with gcc-4.8 as default, I found that the cppunit tests complained that C++11 is required for the stuff they do. Based on experience with Travis CI environments, I know that their gcc-4.6 does not have C++11 at all, 4.9 and newer have it as a first-class citizen, and for 4.8 it must be enabled via command-line, with default being an older standard.

The CFLAGS (vs CXXFLAGS) notion came from an existing variable in the file... I started out tweaking its value, than added a separate lever to pull (or undefine altogether) easily, and overlooked the naming.

@jimklimov
Copy link
Copy Markdown
Member Author

jimklimov commented Aug 30, 2017

As for timehead.h - systems do vary with their paths, and some distros may be sloppier than others. I added my comment to #331 on this part now :)

@clepple
Copy link
Copy Markdown
Member

clepple commented Aug 30, 2017 via email

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.

3 participants