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

Upgrade SQLite to 3.29.0 on macOS and Windows #5810

Merged
merged 7 commits into from
Sep 18, 2019

Conversation

zwass
Copy link
Member

@zwass zwass commented Sep 16, 2019

Mitigates recent CVEs in SQLite. It was already this version on Linux.

Mitigates recent CVEs in SQLite. It was already this version on Linux.
@zwass zwass added build cmake pure cmake changes labels Sep 16, 2019
@zwass
Copy link
Member Author

zwass commented Sep 16, 2019

We probably ought to change the layer name from hack to something more descriptive. Maybe source_migration?

@zwass
Copy link
Member Author

zwass commented Sep 16, 2019

Layer name has been changed to source_migration. I believe this is ready to merge if someone approves.

@zwass
Copy link
Member Author

zwass commented Sep 17, 2019

I moved sqlite entirely out of source into source_migration for all platforms. The proposed idea is that we would move every library from source into source_migration as we were able to make them compatible, then eventually replace source with source_migration.

I suppose it might also make sense to remove sqlite from facebook in this PR.

@Smjert
Copy link
Member

Smjert commented Sep 17, 2019

Thanks for looking into this!

I suggest to avoid moving or duplicating files, for a matter of maintenance and git history.
It would be better if the source_migration layer just acts as a proxy toward the source layer of the libraries that are migrated.

In pratice: for each library that also supports macOS and Windows, you mirror its CMake files (Findxxx.cmake and its CMakeLists.txt) in the new source_migration folder, but they actually include() the CMake files of the source layer and nothing else.
This way we don't need any moving or duplication and when we are done we just delete/drop the source_migration layer.
Same thing for the utils.cmake file, just include the source layer one.

@zwass
Copy link
Member Author

zwass commented Sep 17, 2019

Updated as per comment by @Smjert to include the file in the source layer, and make the appropriate changes there.

@Smjert
Copy link
Member

Smjert commented Sep 17, 2019

The changes look good, though it would be better if the silencing of warnings coming from the third party libraries is reproduced also for Windows.

@zwass
Copy link
Member Author

zwass commented Sep 18, 2019

I added the warning suppression for Windows as well. This does introduce a new warning due to /W3 being overridden by /w. I spent hours trying to resolve it by being able to remove /W3 from the third party targets, but ultimately I think it requires a refactor of how the options are set that should be handled separately from this PR.

@Smjert
Copy link
Member

Smjert commented Sep 18, 2019

I think it requires a refactor of how the options are set that should be handled separately from this PR.

What you mean here?
The way things are layed out are quite intentional, especially for third party libraries and submodules, where you want those extra flags to be put as last in the command line and also solve some of the variable/target scopes issues that exists due to the CMakeLists.txt position in the hierarchy.

If your intention is to solve the /W3 issue, why not just replicate what the policy does?
You can simply remove that /W3 by changing the contents of CMAKE_C_FLAGS and CMAKE_CXX_FLAGS in the flags.cmake file, and then re-add it only on the targets you want.
Namely, on the bottom of flags.cmake you have:

osquery/cmake/flags.cmake

Lines 277 to 293 in 65aca42

add_library(osquery_cxx_settings INTERFACE)
target_link_libraries(osquery_cxx_settings INTERFACE
cxx_settings
)
target_compile_definitions(osquery_cxx_settings INTERFACE
${osquery_defines}
)
add_library(osquery_c_settings INTERFACE)
target_link_libraries(osquery_c_settings INTERFACE
c_settings
)
target_compile_definitions(osquery_c_settings INTERFACE
${osquery_defines}

This is done so that you can choose to have flags that do not touch third party targets (they inherit only from c_settings and cxx_settings).
There you can add /W3 if needed to osquery targets but not to third party ones, and you can keep what you put in this PR.

Or even better, if this is again your "only" issue, is to update the minimum required CMake version to the latest stable.
We don't find this an issue as it may sound because CMake is pretty easy to obtain already built, and I've successfully used it till now on CentOS 6 or if you really need to build it, again it's as easy as run a single script they provide to build it (or you can use a recent enough CMake you have in the system if you prefer).

@zwass
Copy link
Member Author

zwass commented Sep 18, 2019

Turns out it was all unnecessary anyway... If you just set /W0 (which has the same meaning) instead of /w, you don't get an error about repeated flags. If this passes on CI now, it should be good to merge.

@zwass zwass merged commit 5cec1f4 into osquery:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build cmake pure cmake changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants