-
-
Notifications
You must be signed in to change notification settings - Fork 71
option to disable the '#' flag #288
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
Closed
stefano-zanotti-88
wants to merge
5
commits into
charlesnicholson:main
from
stefano-zanotti-88:optional_alt_form
Closed
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7b97e23
option to disable the '#' flag
stefano-zanotti 9d18844
canonicalized some #if checks
stefano-zanotti 37dc88b
windows + mac download (#300)
stefano-zanotti 850bab7
changed NANOPRINTF_USE_ALT_FORM_MODIFIER to the better NANOPRINTF_USE…
stefano-zanotti 890e952
made NANOPRINTF_USE_ALT_FORM_FLAG known to the build system
stefano-zanotti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Please change these tests to
#if NANOPRINTF_USE_ALT_FORM_MODIFIER == 1(For better or worse, the rest of the file uses the== 1convention so I'd prefer to preserve it, and if it does change, do a sweeping change all at once).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.
I wrote "Removing it --ie still parsing it, but ignoring it--", whereas the actual code I committed skips the parsing too.
I'm not sure if you prefer the "parse but ignore" or the "fail parsing" route.
As I've commented elsewhere, we could also consider a different option for "everything that is disabled will not be honored, but will still be parsed".
I've corrected the code as you asked.
Also: if you find it easier to just take the code, tweak it, and commit it in your own branch, without having to accept the PR --which might require slower back and forth discussions with me--, that's perfectly fine. No need for changes to the contributors list or merging from my fork.
Finally, I have problems with running all the tests as specified in the readme, so it would be great if you could help me out here.
Running the build.py script, I get an HTTP error. I can give more details if you can help.
I don't understand how to build the tests myself, so right now I'm just working with a reduced set of tests in a separate file of my own.
Even if I don't use the python build, what am I supposed to compile all the "unit*.cc", "conformance.cc", "doctest_main.cc", and "paland.cc" ? All of them together don't compile. I managed to compile some of them, but the tests fail because of some conformance checks that compare implementation-defined behavior against my local standard library, which is not guaranteed to behave like NPF.
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.
Sure, I'm happy to take this as a proof of concept and do it via a local PR at my bench if that's easier for you. I can do all the test work as well if you'd like.
Nanoprintf uses a python script to kick off CMake to generate a ninja build that compiles the 2^N flag configuration explosion. It's more than I'd do for a less-configurable project, but if nanoprintf offers a flag then I need to test that every possible build combination works with or without that flag. So, the CMakeLists.txt file has a large and very nested loop for each flag, and emits a configuration target with that particular combination of flags.
You should just be able to clone npf and run
./b --paland -v- the python script looks for cmake and ninja in your system and, if they're absent, downloads them to the local transient "build" directory in your repo directory. (It doesn't attempt to install them or anything rude). Then, having either found or downloaded them, it simply runs cmake to configure and then ninja to build.If that's not working for you, it might be a bug or platform incompatibility in the python script- I only test that it works on linux, so if you're on a different platform then it may not even work. (Again, the philosophy of "if it's not harnessed under CI, it's probably broken" shows itself!)
If you have the time and gumption, could I ask you to open an issue with output so that I can take a look? Generally most clients simply use nanoprintf as a submodule or just grab the .h file -- that's what it's there for -- so the build and test machinery has been lower priority. I'd still like it to work, though!
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.
Yes, it's fine if you work on your local PR.
I'll still try to run the tests, as that can speed things up.
I'm having trouble with the non-existent:
https://cmake.org/files/v3.22/cmake-3.22.2-win64-x86_64.tar.gz
which makes the build script fail.
I'll try and download cmake manually when I have time, and go on from there.
Do you think cmake 3.31 vs 4.0 should make any difference?
I'm on Windows, by the way. So I ignore "b" and run
python build.pyThere 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.
I just landed a PR that tests windows downloading and fixed a few issues- cmake changed its download file naming scheme and extension for windows, which explains and fixes the error you saw.
So,
python build.py --download --paland -vwill force the downloading and sandboxing of the latest ninja + cmake versions, and give you as close of a local experience as possible to what the build server is doing.If you're curious, you can go here: https://github.com/charlesnicholson/nanoprintf/actions/runs/14815979224/job/41596450464?pr=300
and expand the "Build" tab to see what it's doing. Hopefully it will be the same on your computer? Please let me know if you try it out.
I'll start the PR integration work tomorrow, by the way, thanks again for all the PRs and very thoughtful discussion!
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.
I'm running
python build.pyfrom a cmd prompt. I have MinGW "installed" (placed somewhere reachable from the system PATH).I just tried building from git bash, and the error is the same -- it is still finding MinGW and compiling with that.
I don't know how to run it "inside of a visual studio developer command prompt". Do you mean running
python build.pyin the console of a VS code installation, or of full Visual Studio? I'm not familiar with that, but I could try. Or I could download clang and get the build script to find that instead of MinGW's GCC.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.
Fascinating- sorry you're going through this; you're challenging a bunch of my assumptions about windows development :) I was in videogames + windows app development for a decade or so, and having a full Visual Studio installation was something that everybody always had for any software projects, so I'm clearly taking that for granted.
When you have Visual Studio installed for C/C++ development, it installs a shortcut that will launch powershell or the command prompt with various environment variables set up. Those update your PATH to include directories that contain programs like "cl.exe" and "link.exe" which are used for command-line compilation via MSVC. When those path entries are present, CMake will discover those tools during its host-platform-interrogation phase, and say "oh, I see, I'm doing a visual studio build." You can see CI run the script that sets the environment up for 32- or 64-bit compilation mode here: https://github.com/charlesnicholson/nanoprintf/blob/main/.github/workflows/presubmit.yml#L196
There's no particular reason that nanoprintf shouldn't work with mingw-based setups; I just haven't done the work to set it up because it's not how I develop when I'm on windows!
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.
I had a VS Code lying around from 2020, with the ms-vscode.cpptools extension installed.
I tried building from its terminal but it still finds MinGW. I hoped it would come with the necessary tools you described, but apparently that's only with full VS.
I'll see if I can get it, though it was already really heavyweight the last time I installed it years ago.
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.
I've never tried it through VS Code; I always do the (yes, really heavyweight) full VS installation.
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.
VS's 26GB are really too much for my home connection.
Instead, I downloaded clang from here: https://github.com/llvm/llvm-project/releases
And I got CMake to find it instead, despite weird problems with Windows cmd.
However, now I get the error
I find it strange that this clang invocation gets the "-std=gnu++17" option rather than "-std=c++17". I can't find such an option in CMakeLists.txt
I tried adding
add_compile_options("-std=c++17")somewhere in CMakeLists.txt, and it is indeed used now, but the error remains the same.Also strange: doctest.h:978 is a line of code guarded by
#if defined(_MSC_VER) && _MSC_VER <= 1900, so why is it being compiled by clang?Building from cmd and from git bash is identical: it uses clang, and produces this error.
There is also a
--dependent-lib=msvcrtin the compilation command. Could it be a cause/symptom of the problem?