-
Notifications
You must be signed in to change notification settings - Fork 7
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 CMake build system and and updated README.md #48
base: master
Are you sure you want to change the base?
Conversation
jopadan
commented
Mar 28, 2022
•
edited
Loading
edited
- add CMake build and restructure source tree
- adapt script_param_to_(u)int to 64-bit platforms
@jopadan did you close and make a new issue again? there is no reason to do this and only makes the list of closed PR's more messy, and makes it harder to see what the latest changes are, and makes it difficult to follow what has already been discussed on the PR, could you please just do everything in the same PR |
I concur, please stop opening new pull request for the same thing. It's going to be squashed anyways, so it's entirely a waste of time trying to make the git log "pretty". If you must insist on making the git log pretty, just force push your own branch if you must. Anyways
CMakeOutput.log:
|
find_package( SDL REQUIRED ) | ||
target_link_libraries( ${ARG_NAME} PUBLIC ${SDL_LIBRARY} ) | ||
target_compile_definitions( ${ARG_NAME} PUBLIC ENTRY_CONFIG_USE_SDL ) | ||
target_link_libraries( ${ARG_NAME} PUBLIC X11 Math::Math ) |
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.
Any reason for doing Math::Math
instead of just m
? Seems to work fine just using m
- I made a pull request to your repo with the change: jopadan#1
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.
You need to install libadikted first and run
export PKG_CONFIG_PATH=$PREFIX/${libdir}/pkgconfig && ldconfig
I apologize for reopening the pull request.
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.
Alright, fixing mapslang compile issue can wait I'd rather have this PR merged sooner rather than later and invoking cmake from the root directory works just fine for now. I'll do a proper review when I get home from work tomorrow. Good work though, I appreciate it!
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 propose a package interface named DKFans::ADiKtED and resolve the dependencies for parent directories too.
There are possible non -lm math library linker flags in existence.
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.
Some super minor things to changes left, but looks good to me otherwise.
#### CMake | ||
|
||
Run | ||
`cmake --install-prefix=/usr . && make install` |
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.
Change to cmake . && cmake --build . && cmake --install .
as CMake may have used a different generator that doesn't create a Makefile. For example:
➜ ADiKtEd git:(pr-48) ✗ CMAKE_GENERATOR=Ninja
➜ ADiKtEd git:(pr-48) ✗ cmake --install-prefix=/usr . && make install
-- Configuring done
-- Generating done
-- Build files have been written to: /home/dotted/ADiKtEd
make: *** No rule to make target 'install'. Stop.
I also don't think we should be suggesting /usr
as a prefix, as in general /usr/bin
refers to the folder where the OS installs its managed binaries. /usr/local
is the proper prefix to use for this type of binary, and is also the default used by CMake.
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 was not able to reproduce the error using Linux x86_64.
Please be more specific about the host environment.
@jopadan nobody uses adikted anymore, wouldn't waste to much time on this |