Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

METRON-1334 Add C++11 Compliance Check to 'platform-info.sh' #849

Closed
wants to merge 3 commits into from

Conversation

nickwallen
Copy link
Contributor

@nickwallen nickwallen commented Nov 27, 2017

Some of the module dependencies for the Management and Alerts UI must be built natively on the host. This requires a C/C++ compiler. In addition, some of the dependencies require a C++11 compliant compiler. This is causing problems for users who attempt to build Metron on a system with an older version of GCC, like CentOS 6.

Not having a C++11 compliant compiler can cause some non-obvious error messages when the build fails. This adds a version check for GCC and also a C++11 compliance check. The compiler itself must be on the user's PATH, which is what the Node modules also require.

If the compiler is C++11 compliant, you will see the following.

--
g++
g++ (GCC) 5.3.1 20160406 (Red Hat 5.3.1-6)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

--
Compiler is C++11 compliant

If the compiler is NOT C++11 compliant, you will see the following.

--
g++
g++ (GCC) 4.4.7 20120313 (Red Hat 4.4.7-18)
Copyright (C) 2010 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

--
cc1plus: error: unrecognized command line option "-std=c++11"
Warning: Compiler is NOT C++11 compliant

This was tested manually on Mac OS X, CentOS 6, CentOS 7, and Ubuntu Trusty 64.

Pull Request Checklist

  • Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
  • Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Have you included steps to reproduce the behavior or problem that is being changed or addressed?
  • Have you included steps or a guide to how the change may be verified and tested manually?
  • Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
  • Have you written or updated unit tests and or integration tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

@nickwallen nickwallen changed the title METRON-1320 Add C++11 Compliance Check to platform-info METRON-1334 Add C++11 Compliance Check to 'platform-info.sh' Nov 27, 2017
else
echo "Warning: Compiler is NOT C++11 compliant"
fi
rm -f $CPPFILE $OBJFILE

Copy link
Contributor

Choose a reason for hiding this comment

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

A standard way in the language is to check for the value of __cplusplus

201402L is CPP 14
201103L is CPP 11

I don't have a preference, just throwing this out there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to account for platform_info on a non-dev machine? If there is no g++ at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have to account for platform_info on a non-dev machine? If there is no g++ at all?

The platform-info script is there to tell us whether a machine can be used to deploy metron. Right now, you have to build Metron to deploy it, so the script checks all dev dependencies.

If g++ is not in the path, then the script will tell us that. And it would carry on its merry way. That's the behavior that I would expect.

Maybe I am not following you. Can you describe the scenario you are thinking of more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A standard way in the language is to check for the value of __cplusplus

Thanks, I saw that. I didn't think it was worth the trouble. IMHO

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree about it not being worth the trouble.

as far as g++, I would expect us to check (pseudo)

if c++ in path then
build test program
else
echo No Cpp, you are in trouble!
fi

Maybe I'm mis-reading this, if there is no cpp, it will still try to build etc won't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I ran it, I did mis-understand. sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. Glad to have a second set of eyes on it.

@ottobackwards
Copy link
Contributor

+1, Tested on OSX and on centos 6 with and without compliant g++ installed
Thanks Nick!

@JonZeolla
Copy link
Member

Ran on a purposefully misconfigured macOS system. Is this the expected output?

--
g++
xcrun: error: invalid active developer path (/Library/Developer/CommandLineTools), missing xcrun at: /Library/Developer/CommandLineTools/usr/bin/xcrun
--
xcrun: error: invalid active developer path (/Library/Developer/CommandLineTools), missing xcrun at: /Library/Developer/CommandLineTools/usr/bin/xcrun
Warning: Compiler is NOT C++11 compliant
--

@ottobackwards
Copy link
Contributor

I think that is valid

@nickwallen
Copy link
Contributor Author

Is this the expected output?

I agree with @ottobackwards . That looks like what I would expect to see.

@JonZeolla I assume since you asked, it is not what you would expect. What would prefer to see here?

@JonZeolla
Copy link
Member

JonZeolla commented Nov 29, 2017

Instead of trying to explaining my position, I threw together a quick PR to illustrate my thoughts. Feel free to take another approach if you'd prefer, or merge and/or modify without attribution. I tested it with g++ configured, unconfigured, and not in my PATH on macOS, as well as on a CentOS 7 box with and without gcc installed, and an Ubuntu 14.04 box with an old version of gcc installed (not C++11 compliant). All worked as expected.

@JonZeolla
Copy link
Member

JonZeolla commented Dec 1, 2017

+1 great stuff @nickwallen.

@asfgit asfgit closed this in 05cb59b Dec 2, 2017
iraghumitra pushed a commit to iraghumitra/incubator-metron that referenced this pull request Feb 17, 2018
@nickwallen nickwallen deleted the METRON-1320 branch September 17, 2018 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants