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

Match CMakeOptions.txt options default values #3258

Merged
merged 1 commit into from Aug 26, 2023
Merged

Match CMakeOptions.txt options default values #3258

merged 1 commit into from Aug 26, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 22, 2023

No description provided.

@orcmid
Copy link
Contributor

orcmid commented Aug 22, 2023

I am in no position to assess the wisdom of that.

However, an alternative is to simply have those in CMake as previously specified but to also define EXTERNAL_CONFIG_FLAGS in that list. The CMake ON setting will go through and for the OFF (unset?) ones, whatever the files do in the absence of config.h and particular CMAKE setting will kick in.

I have no idea what kind of edge cases/holes might sneak in with that approach, though.

I think the expectation is that if neither CMAKE (nor command line) nor config.h are used to turn on a preprocessor var, files will detect what definitions have not been provided and automatically establish the official defaults stated in the (default) config.h file. That's much to check/test though.

@orcmid
Copy link
Contributor

orcmid commented Aug 22, 2023

If you remove the CMakeOptions.txt from CMake, config.h will be used always and that is then the one place to make changes, whether using make or CMake. I think that is what Ray has in mind for it all to work.

Ultimately, if you want to change those choices, you have to change something in the installed raylib library. It seems that is what config.h is for.

config.h probably needs to be in a .gitignore in the raylib repo but it gets tricky how to have it be distributed in a release and in a fork. It can be done, but it means the user-changeable config.h needs to be copied from a different default value, such as a config.h.default. That's the only technique I have managed to use that prevents poisoning a clone/fork.

That makes things a little trickier for neophytes though. It does seem better than having these mysterious conflicts between CMake and make builds and other problems with "poisoning" CMakeOptions.txt instead :).

@orcmid
Copy link
Contributor

orcmid commented Aug 22, 2023

I think I have misled you somehow. My suggestion is that CMakeOptions.txt be removed from the raylib CMake, in the raylib repo. That way, there will be no conflict with config.h which should be left as-is.

There's also something that needs to be done about documenting config.h and how an user can modify that in the way that Ramon intends. I guess if an user does that from a raylib fork/clone, they need to do it in a branch that is never pushed, so the raylib repo does not become polluted. If installation of raylib is from a source release, no problem.

I mentioned other safeguards that can be done instead, but that may be too complicated.

@raysan5
Copy link
Owner

raysan5 commented Aug 26, 2023

@asdqwe thanks for the review, alighning CMakeOptions.txt with config.h is a good start.

If you remove the CMakeOptions.txt from CMake, config.h will be used always and that is then the one place to make changes, whether using make or CMake. I think that is what Ray has in mind for it all to work.

I don't maintain the CMake build system but it think keeping a CMakeOptions.txt available to modify build options is the simples option for CMake users, instead of directly editing a source code file. Personally, I use similar approaches with Makefile (gcc) and Directory.Build.props (VS2022) build files... and that was the best solution I found.

The problem with those mechanisms is that they could be miss-aligned easely... but still, that's ok, build configuration is up to the user, independently of default values.

I'm also considering to reduce the number of build options, just noticed that some options are always required and the overhead added is really minimal (i.e. compress/decompress functionality).

@raysan5 raysan5 merged commit 8f22862 into raysan5:master Aug 26, 2023
@ghost ghost deleted the fix/cmake-options branch August 27, 2023 01:43
@raysan5
Copy link
Owner

raysan5 commented Aug 27, 2023

@asdqwe Yeah, those flags should be added to CMakeOptions.txt. Feel free to send a PR with that addition.

Please note that I'm not the maintainer of CMake build system, it was added for convenience by some user. Consequently, it could not be so regularly updated like the other available build systems that I manage (Makefile, Visual Studio projects).

@ghost ghost mentioned this pull request Aug 27, 2023
@orcmid
Copy link
Contributor

orcmid commented Aug 27, 2023

I think I've said this already, but I'll try one more time.

Anything CMake does not set will be handled by config.h. The config.h header plays nice and only introduces its settings for preprocessor macros that are not already defined. That also works no matter what build system is used. There is a preprocessor variable, that if set in CMake, suppresses all use of config.h.

I recommend that config.h be the file of choice for varying the particular settings. And if it is awkward editing something from a raylib release, then set up a build in a way that introduces settings different from those that config.h defaults in some way that does not involve altering files of the raylib src release, whether config.h or CMake.

The problem with this CMake/config.h duality it puts beginners at risk of an IED trap when a modification to config.h doesn't change anything and it is not understood that CMake has trumped it.

Most of all, I think there needs to be more understanding of config.h and how it can be over-ridden from afar. I'm not the one to do that unless Ramon thinks that is a good direction to take.

@raysan5
Copy link
Owner

raysan5 commented Aug 27, 2023

@orcmid I think it should probably be better documented, but, in any case, users tweaking it (or the related build options) should be advance users with some knowledge of build systems.

raysan5 added a commit that referenced this pull request Sep 17, 2023
* Prettified a comment

* fixed broken indentation caused by another commit.
the commit renamed a bool to int and broke indentation: 233cf39

* Changed 0.001 and 0.00001 to EPSILON
This commit is untested.
I don't know what consequences this has.
Since the commits that added these numbers were before epsilon was added,
I have assumed that epsilon could replace them.

* Prettied up indentation in a few places

* removed spacing around *, standardizing it.

* I may have gotten overboard with indentation

* removed a few useless parenthesis

* Added fortran-raylib

* Fix examples/others/rlgl_standalone.c compilation issue (#3242)

* Update BINDINGS.md

* Ignore unused return value of GetCodepointNext in GetCodepointCount (#3241)

* Ignore unused return value of GetCodepointNext in GetCodepointCount

Removes the last warning from non-external libraries when compiling with
the default build configuration on x64 Linux.

* Remove unnecessary void cast in GetCodepointCount

* Fix #3246

* Revert "Fix #3246"

This reverts commit e4dcbd5.

* Fix text_unicode.c example crashing (#3250)

* Fix text_unicode.c example crashing

* Adjust the text_unicode.c example crashing fix

* tweaks

* add build.zig options for individual modules (#3254)

* Add `IsKeyPressedRepeat` (desktop only) (#3245)

Since the key pressed are handle by comparing current vs previous
state (ie frame), a special way is needed to handle key repeats.

* Reviewed `IsKeyPressedRepeat()` #3248

* Update rcore.c (#3255)

* Match CMakeOptions.txt options default values (#3258)

* Fix SetClipboardText for web (#3257)

* [Image] Validate that ImageDrawRectangleRec is drawing entirely inside the image (#3264)

* Add a function to clone a sound and share data with another sound.

* rename items based on feedback

* PR Feedback, use custom unload for sound alias, not variant of normal sound unloading

* sound_multi example

* Validate that image rect drawing is inside the image so we don't overflow a buffer

* remove files that should not have been added.

* remove changes that should not have been

* revert

* adsfasdfsdfsdf

* Add Vector3 Projecting and Rejection to Raymath (#3263)

* Update raymath.h

* formatting

* [Feature] IsKey... safety checks and more (#3256)

* [Feature] Add GetKeyRepeat

* Update rcore.c

* Simpler design, only one repeat per frame

* Update config.h

* Update rcore.c

* Add KEYBOARD_KEYS_MASK

* Update config.h

* reversions

* Update rcore.c

* Update rcore.c

* change docs

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update raylib.h

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Update rcore.c

* Fix bug where default shaders was not linking. (#3261)

* Formating review

* Add missing cmake options (#3267)

* Fix CMake extraneous -lglfw (#3266)

Closes #3265.

The problem: LIBS_PRIVATE is a list of library names (used by pkg-config), but the shared library of the same name doesn't always exist.

* Fix example/models/models_loading_gltf.c controls (#3268)

* Fix example/models/models_loading_m3d.c controls (#3269)

* Remove e from secondes (#3270)

* Fix example/audio/audio_module_player.c help instructions and small bug (#3272)

* Fix example/audio/audio_module_player.c help instructions and small bug

* Update example/audio/audio_module_player.png screenshot

* Use type name instead of valid specifier

long long --> long long int

* REVIEWED: `GetFileLength()`, added comment #3262

* Update examples/models/models_loading_gltf.png;m3d.png screenshots (#3273)

* Remove a duplicated screenshot and add missing one (#3275)

* Add examples/shaders/shaders_lightmap.c to Makefiles (#3276)

* Fix examples/others/easings_testbed.c help instructions and small tweak (#3277)

* Fix examples/shaders/shaders_texture_outline.c help instructions (#3278)

* Fix examples/shapes/shapes_collision_area.c help instructions (#3279)

* RENAMED: LoadFont*() parameter names for consistency and coherence

* Fix uninitialized thread-locals in stbi #3282 (#3283)

* REVIEWED: Added `SetTextLineSpacing()` to multiline examples

* REVIEWED: Data size type consistency between functions #3168

* Some tweaks

* Use internal default allocators, instead of user-exposed ones

* Added rudimentary SVG support. (#2738)

* Added rudimentary SVG support. Added 2 functions ImageLoadSvg and ImageLoadSvgWithSize.

* Added an example on how to use ImageLoadSvgWithSize and adjusted Makefiles accordingly.

* Added actual correct example file.

* Reviewed the code to keep the raylib coding conventions in mind.
Moved the LoadImageSvg() code into LoadImage() guarded by SUPPORT_FILEFORMAT_SVG.
Renamed LoadImageSvgWithSize() to LoadImageSvg().
Added a LoadImageSvgFromString() function to parse the loaded SVG into an actual image. This does the bulk of the work.

* Fixed typo.

---------

Co-authored-by: Ray <[email protected]>

* REVIEWED: `LoadImageSvg()`

* REVIEWED: `LoadImageSvg()`

* Add SUPPORT_FILEFORMAT_SVG to cmake (#3284)

* Fix examples/textures/textures_fog_of_war.c help instructions (#3285)

* Fix examples/textures/textures_image_rotate.c help instructions (#3286)

* Update rtextures.c

* Fix #3247

* Update config.h

* Fix #3293

* Disable UBSAN in zig builds. (#3292)

Zig debug builds automatically enable ubsan.
As the fix for #1891 had to be reverted, debug builds using zig will crash like so:

```
Illegal instruction at address 0x3237d2
raylib/src/rlgl.h:3690:91: 0x3237d2 in rlDrawVertexArrayElements (/home/rcorre/src/raylib-zig-template/raylib/src/rcore.c)
    glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_SHORT, (const unsigned short *)buffer + offset);
```

This disables UBSAN when using zig to build raylib.

* Update README.md (#3290)

specially -> especially

* Update cmake SUPPORT_FILEFORMAT_SVG default value (#3291)

* Mouse offset and scaling must be considered also on web!

* Update rcore.c

* Update Makefile : clean raygui.c & physac.c (#3296)

* Remove PLATFORM_RPI (#3232)

* Remove PLATFORM_RPI

* remove build artifacts

---------

Co-authored-by: MichaelFiber <[email protected]>
Co-authored-by: Ray <[email protected]>

* Review to avoid UBSAN complaining #1891

* added raylib-raku to bindings (#3299)

* examples: core: adds 2D camera two player split screen (#3298)

* Reviewed examples for consistency

* Update rtext.c

* Some code restructuring for input functions, consistency review

* Remove unneeded #if (#3301)

Co-authored-by: MichaelFiber <[email protected]>

* Revert "Disable UBSAN in zig builds. (#3292)" (#3303)

This reverts commit a316f9e.

Issue #1891 was fixed again, so this is no longer needed.

* rtextures: Fix ImageDraw() source clipping when drawing beyond top left (#3306)

* REVIEWED: `TextToPascal()` issue when first char is uppercase

* Implement FLAG_WINDOW_RESIZABLE for web (#3305)

Fixes #3231

* Update BINDINGS.md (#3307)

Fix Kaylib binding. Reroute to a new repository.
Binding renamed.

* Update webassembly.yml

* Add claw-raylib to BINDINGS.md (#3310)

* Add SetWindowMaxSize for desktop and web (#3309)

* Add SetWindowMaxSize for desktop and web

* Remove SizeInt and respective adjustments

* Update rtextures.c

* Reviewed parameters for consistency

* Rename windowM* to screenM* (#3312)

* Update BINDINGS.md (#3317)

Update TurboRaylib bindings

* Update rmodels.c

* Update BINDINGS.md with vaiorabbit/raylib-bindings (#3318)

* fixed spelling mistake

* put back parenthesis

* reverted major allignment changes

* reverted parser output changes

* reverted one more indentation change

---------

Co-authored-by: Brian-E <[email protected]>
Co-authored-by: Ray <[email protected]>
Co-authored-by: ubkp <[email protected]>
Co-authored-by: ashn <[email protected]>
Co-authored-by: actondev (Christos) <[email protected]>
Co-authored-by: vitopigno <[email protected]>
Co-authored-by: Asdqwe <[email protected]>
Co-authored-by: Jeffery Myers <[email protected]>
Co-authored-by: Ethan Simpson <[email protected]>
Co-authored-by: Nickolas McDonald <[email protected]>
Co-authored-by: Branimir Ričko <[email protected]>
Co-authored-by: iacore <[email protected]>
Co-authored-by: Ethan Conneely <[email protected]>
Co-authored-by: Johannes Barthelmes <[email protected]>
Co-authored-by: bXi <[email protected]>
Co-authored-by: Ryan Roden-Corrent <[email protected]>
Co-authored-by: Ikko Eltociear Ashimine <[email protected]>
Co-authored-by: SuperUserNameMan <[email protected]>
Co-authored-by: MichaelFiber <[email protected]>
Co-authored-by: MichaelFiber <[email protected]>
Co-authored-by: Dan Vu <[email protected]>
Co-authored-by: Gabriel dos Santos Sanches <[email protected]>
Co-authored-by: Rob Loach <[email protected]>
Co-authored-by: Peter0x44 <[email protected]>
Co-authored-by: Kenta <[email protected]>
Co-authored-by: bohonghuang <[email protected]>
Co-authored-by: turborium <[email protected]>
Co-authored-by: Wilson Silva <[email protected]>
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.

3 participants