-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add a test for version macros/functions #60
Add a test for version macros/functions #60
Conversation
I don't think SDL needs to be initialized in order to be able to access the |
b8ba88f
to
f6d54e5
Compare
You're right, thanks! EDIT: The issue I was facing with XDG_RUNTIME_DIR has to be solved in the future though, as other tests will need SDL to be initilized. |
tests/testversion.pas
Outdated
|
||
try | ||
if (SDL_VERSIONNUM(1,2,3) <> 1203) then | ||
raise ESDL2Error.Create('SDL_VERSIONNUM failed: 1203 expected, found: ' + IntToStr(SDL_VERSIONNUM(1,2,3))); |
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.
- Consider assigning
SDL_VERSIONNUM()
to a variable, instead of calculating it twice. - Consider using
WriteStr()
here.
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 wonder, what would be the benefit of WriteStr here?
Best regards
tests/testversion.pas
Outdated
if (SDL_COMPILEDVERSION <> SDL_VERSIONNUM(CompiledVersion.major, CompiledVersion.minor, CompiledVersion.patch)) then | ||
raise ESDL2Error.Create('SDL_VERSION or SDL_COMPILEDVERSION failed: Version results do not match!'); | ||
|
||
if (SDL_VERSION_ATLEAST(2,0,0) <> True) then |
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.
Personally I really dislike comparisons with boolean literals (unless we're talking about dynamically-typed languages). I'd simplify this to just:
if not SDL_VERSION_ATLEAST(2,0,0) then
tests/testversion.pas
Outdated
if (SDL_VERSION_ATLEAST(2,0,0) <> True) then | ||
raise ESDL2Error.Create('SDL_VERSION_ATLEAST failed: Version at least 2.0.0 should be true!'); | ||
|
||
if (SDL_VERSION_ATLEAST(99,99,99) <> False) then |
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.
Similarly here.
if SDL_VERSION_ATLEAST(99,99,99)
Also, (99,99,99)
seems excessive. A check against (3,0,0)
should suffice.
uses: suve/[email protected] | ||
with: | ||
source: tests/testversion.pas | ||
flags: Fl/usr/local/lib |
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.
This shouldn't be required.
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.
Why? - Without this part testversion.pas is not compiled, right?
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 mean strictly this line, where -Fl/usr/local/lib
is passed to the compiler. The SDL packages installed from the repo should install libraries in /usr/lib
, which is one of the locations where the compiler looks by default.
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.
The reason why it is there, is that without this line the sdl2 library is not found. From my memory, it is related to a new way of storing libraries on big sur, they are in fact not some files stored in a folder anymore but in a more sophisticated indexed "library file".
See. 8f7b010
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.
Ah, sorry, I missed that this is part of the Mac job. I suppose that since brew
is not the official package manager, the authors decided to put stuff in /usr/local
over /usr
.
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 see :)
Thanks for your hints! I added some patches. About the CI-script though, see above, pls. |
This test results from a examination of issue #30 . - And as it now exists, it can be added to the CI tests.