-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix minor issues in MSVC projects. #3333
Conversation
Thanks for you contribution. Unfortunately this fails our CI:
|
We're still getting a test failure in one of our configs:
|
In my opinion a more straightforward message saying generated file differs from the one in the library might have been more clear than |
Hi! Apologies, this is admittedly slightly confusing. Only the template files ( Once you have done this, tests/scripts/check-generated-files.sh should run without error (it will moan if the generated version doesn't match the version in a given changelist). |
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.
LGTM
Thank you, @paul-elliott-arm By the way, |
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.
Just a couple of minor things:
-
Please add a ChangeLog entry.
-
Would be nice to add a "Fixes A bug in .vcxpoj files and suggested fix #3297" (or similar) to the commit message.
defaults. Fixes Mbed-TLS#3297. Signed-off-by: irwir <[email protected]>
08ad037
to
fab9373
Compare
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.
LGTM
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.
Sorry, I'm withdrawing my approval as I've just spotted a CI failure. The ChangeLog didn't assemble:
******************************************************************
* check_changelog: Check: changelog entries
* Tue Jun 9 03:26:32 UTC 2020
******************************************************************
Traceback (most recent call last):
File "scripts/assemble_changelog.py", line 505, in <module>
main()
File "scripts/assemble_changelog.py", line 502, in main
merge_entries(options)
File "scripts/assemble_changelog.py", line 449, in merge_entries
finish_output(changelog, options.output, options.input, files_to_merge)
File "scripts/assemble_changelog.py", line 415, in finish_output
check_output(output_temp, input_file, merged_files)
File "scripts/assemble_changelog.py", line 399, in check_output
raise LostContent(merged_file, line)
__main__.LostContent: Lost content from ChangeLog.d/bugfix_PR3333.txt: "b' * Remove unused macros from MSVC projects. Reported in #3297 and fix submitted in #3333 by irwir.'"
^^^^check_changelog: Check: changelog entries: scripts/assemble_changelog.py -o ChangeLog.new -> 1^^^^
It seems the ChangeLog script can't handle the lack of whitespace at the end of the this ChangeLog entry. We will look into fixing the script but if you want this merge in the meantime, just add a LF to the entry.
Signed-off-by: irwir <[email protected]>
Text files need a LF at the end. The lack of a LF at the end of a changelog entry files breaks the changelog assembly file with a hard-to-understand error. That's low-priority because it's about dealing with ill-formed input that
and fixing this will make |
I'm taking Paul's previous approval since there was only the LF change to the ChangeLog entry. All CI passing now. |
Description
Resolves #3297 - erroneous definition is removed.
Removed preprocessor definitions from project files:
as unused -
_WINDOWS
WIN32
WIN64
as predefined -
_DEBUG
Removed explicit settings with default values:
PrecompiledHeader
andShowProgress
Status
READY
Requires Backporting
NO, but might be considered.
Which branch?
development
Migrations
NO
Additional comments
I left for now, but may also delete
_CONSOLE
.This is never referenced anywhere in the repository and probably is not useful in the example programs.