-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 SmartOS build #1017 #1018
Fix SmartOS build #1017 #1018
Conversation
Signed-off-by: Dan Fredell <[email protected]>
I have actually 0 experience with smartos and last time I touched solaris was 10 years ago.. Are you sure this doesn't break other solarises? I don't know why these lines were added before. |
I'm not sure if it breaks others, it fixes it for me though. I can now run the exporter and get the metrics.
My little research points to golang/go#23749 which is a security change in golang the prevents unknown gcc flags. |
@kpettijohn You've added this collector. Can you confirm that removing this doesn't break other solarises? |
These cgo flags might have been required for a specific GCC compiler provided with a specific SunOS platform. They should not be hardcoded here, because A) -fno-stack-protector is a compiler flag and not on the linker flag whitelist in golang (recent security measures made to tackle golang/go#23672); and B) only compilers should set these if requested at compile time, as SunOS platforms, compilers and user security preferences may differ. |
@mamash thanks for the additional information! When making the original PR I was using SmartOS ( @discordianfish sorry for not jumping in sooner but based on @mamash comments it sounds like dropping the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the feedback. That sounds reasonable, so LGTM.
@SuperQ wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Dan Fredell <[email protected]>
This is a fix for issue #1017
@discordianfish You reviewed my previous SmartOS PR #762 Can you take a look again?
I apologize if I created a pile of notifications trying to get my commit username correct.