Update Build Scripts and README #78
Conversation
Added instructions to the README to include <functional> in socket.hpp for systems using C++11 and later, where unary_function is deprecated.
Updated PowerShell script to reliably detect Visual Studio installations and automatically select the correct CMake generator.
Restored the '-i|--install-all' option in the build script to align with the README documentation.
Added guidance to the README for users experiencing issues with deprecated `unary_function`.
|
|
||
| Run `.\build-windows.ps1` in a new powershell terminal to build the source with support for all available database engines (default: SQL server) and install the project into a local directory (default: `install`) under the repository root. To compile with support for all database engines, run `.\build-windows.ps1 -SqlServer ON -MariaDB ON -Sqlite ON` . `-Debug` can be added to the command-line arguments to build in debug mode, otherwise, release mode is used. | ||
|
|
||
| ### Code Modification |
There was a problem hiding this comment.
@ethanmoffat Why is this note about #include <functional> in the README to begin with - is there any reason not to just add the include unconditionally?
There was a problem hiding this comment.
Agree, this change should be made in the code. Making code changes to a repo you clone should be unnecessary.
If there is an incompatibility with this #include statement between different versions of MSVC, add #ifdef guards that check the cpp/vs version.
| - [Integration Tests](#integration-tests) | ||
| - [Sample servers](#sample-servers) | ||
| - [ETHEOS](#etheos) | ||
| - [Table of Contents](#table-of-contents) |
There was a problem hiding this comment.
- The project name (ETHEOS) is implied by the repo name, and an H1 heading for it is already at the top of the README. A link back to the top is unnecessary.
- A link to the table of contents in the table of contents is unnecessary.
- I don't want the subheadings of "getting started" listed out here, the sections are short enough that I don't think they're necessary.
In short - please back out this change
| ### Build and Install | ||
|
|
||
| Run `.\build-windows.ps1` in a new powershell terminal to build the source with support for all available database engines (default: SQL server) and install the project into a local directory (default: `install`) under the repository root. To compile with support for all database engines, run `.\build-windows.ps1 -SqlServer ON -MariaDB ON -Sqlite ON`. `-Debug` can be added to the command-line arguments to build in debug mode, otherwise, release mode is used. | ||
| Run `.\build-windows.ps1` in a new powershell terminal to build the source with support for all available database engines (default: SQL server) and install the project into a local directory (default: `install`) under the repository root. To compile with support for all database engines, run `.\build-windows.ps1 -SqlServer ON -MariaDB ON -Sqlite ON` . `-Debug` can be added to the command-line arguments to build in debug mode, otherwise, release mode is used. |
There was a problem hiding this comment.
Was there something wrong with the formatting that requires this change? If not, please remove the space so the diff isn't as noisy.
|
|
||
| Run `.\build-windows.ps1` in a new powershell terminal to build the source with support for all available database engines (default: SQL server) and install the project into a local directory (default: `install`) under the repository root. To compile with support for all database engines, run `.\build-windows.ps1 -SqlServer ON -MariaDB ON -Sqlite ON` . `-Debug` can be added to the command-line arguments to build in debug mode, otherwise, release mode is used. | ||
|
|
||
| ### Code Modification |
There was a problem hiding this comment.
Agree, this change should be made in the code. Making code changes to a repo you clone should be unnecessary.
If there is an incompatibility with this #include statement between different versions of MSVC, add #ifdef guards that check the cpp/vs version.
| -t|--test) | ||
| opt_test="true" | ||
| ;; | ||
| -i|--install-all) |
There was a problem hiding this comment.
The intent of my ask for the readme update was to remove the documentation of -i from the README, not add it back to the script. I'd rather the scripts have relative parity between platforms.
Please back this change out, and remove the mention of -i from the README on the linux section.
|
|
||
| $cmakeHelp=$(cmake --help) | ||
|
|
||
| # -requires param : ensure that the visual studio installs have the C++ workload |
There was a problem hiding this comment.
I'm not sure what the purpose of this change to generator selection is. The intent of my ask for the PR was to add a line for support of VS2022 to the enum switch.
Perhaps this was done unintentionally, but it looks like the check for Microsoft.VisualStudio.Component.VC.Tools.x86.x64 has been removed. This is important since some people have VS installed without the C++ tools. It shouldn't try to build with VS if the C++ tools aren't installed, that's the same as not having VS installed at all since the compiler toolchain is missing.
Unless there's a compelling reason for this change I'm missing, please back it out and just add the line for VS2022 that you've been recommending in 404 chat.
|
|
||
| # Validate generator | ||
| if (-not $generator) { | ||
| Write-Error "Unable to determine Visual Studio version. Is Visual Studio installed?" |
There was a problem hiding this comment.
I agree this line needs an update, but please change it to e.g.
Write-Error "Visual Studio not found or missing C++ toolchain."
ethanmoffat
force-pushed
the
master
branch
6 times, most recently
from
a31b13d to
09c5569
Compare
3 participants
This PR addresses inaccuracies in the README and updates scripts