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

In order to protect users of prebid-server that don't use CGO, add build flag #4058

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

scr-oath
Copy link
Contributor

@scr-oath scr-oath commented Nov 18, 2024

Changes between v2.30.0...v2.31.0 force users to use CGO, even if not using the 51degrees module. This change adds the cgo flag to enable bringing in prebid-server without this requirement.

Copy link
Contributor

@jwrosewell jwrosewell left a comment

Choose a reason for hiding this comment

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

Thank you for the additions and looks good with the changes from @justadreamer.

@scr-oath
Copy link
Contributor Author

@bsardo - how can we move this forward?

@SyntaxNode
Copy link
Contributor

@linux019 - You commented on this PR. Do you wish to re-review?

@linux019
Copy link
Contributor

@SyntaxNode
I would use build tags to exclude code which cannot be used without native library. Instead of this we have successfully compiled image which has passed all unit tests but might crash on startup depending on PBS configuration.

If the PBS cannot be run without CGO - don't allow to build the image instead of raising a panic or exclude compiler-depended code
To include module we can use this build tag

//go:build cgo

@scr-oath scr-oath force-pushed the cgo-protect-51degrees branch from fe07dc4 to f996399 Compare December 17, 2024 00:00
@scr-oath scr-oath requested a review from linux019 December 17, 2024 00:11
@scr-oath
Copy link
Contributor Author

@SyntaxNode @linux019 - please take another look - the fiftyonedegrees module is excluded in its entirety now (though, there still needs to be cgo and !cgo files in the modules for the builder to conditionally configure it.

"fiftyonedegrees": {
"devicedetection": fiftyonedegreesDevicedetection.Builder,
},
ret := ModuleBuilders{
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is autogenerated by

go generate modules/modules.go

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.

5 participants