Skip to content

WIP: generalize makefile #241

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

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Conversation

InfiniteVerma
Copy link
Contributor

Description

Generalize makefile by adding variables for compiler, compiler flags, and c files and include directories.
Fixes #125

Motivation and Context

Previously, the makefile had hardcoded c files and paths. Now, with them being listed out in CFILES variable, new files can be added without changes required in the makefile. Also, additional compiler flags can be added by appending them to CFLAGS and SHAREDCFLAGS instead of changing them in all places.

How Has This Been Tested?

Ran make all without errors. Please let me know of any other ways to test this.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Test Coverage (Additional test(s) without any extra code)
  • Documentation Improvements

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes. They do not dramatically decrease the test coverage metric
  • All new and existing tests passed.
  • I give my permission for my code to be used in this project under this license and any future license terms

@InfiniteVerma InfiniteVerma changed the title generalize makefile WIP: generalize makefile Oct 4, 2022
@ianfhunter
Copy link
Owner

This is great, exactly what I was hoping for with that issue 👍

However, it looks like there's some problem with the final files as the automated checks for Python (proper test suite) & Go (basic test) fail.

I can't spot what's wrong from the diff, but it might be worth comparing a before and after of the build directory

Go log:

cp: cannot stat 'build/dice.so': No such file or directory

@ianfhunter ianfhunter enabled auto-merge October 4, 2022 18:21
auto-merge was automatically disabled October 4, 2022 18:37

Head branch was pushed to by a user without write access

@ianfhunter
Copy link
Owner

Great! Passing once more & ready to merge!

@InfiniteVerma
Copy link
Contributor Author

InfiniteVerma commented Oct 4, 2022

@ianfhunter Not sure what's the issue here. Strangely, the failing GitHub action worked the last time.

@ianfhunter
Copy link
Owner

ianfhunter commented Oct 4, 2022

@ianfhunter Not sure what's the issue here. Strangely, the failing GitHub action worked the last time.

The one failing GitHub action is just something to add labels to the PR. There's an issue with it, that it does not work when a pull request is from a fork. Bug Link
Not caused by your code!

I've only seen it recently because of new contributors, been meaning to make it ignore failures

@ianfhunter ianfhunter merged commit 3a9d8ac into ianfhunter:main Oct 4, 2022
@InfiniteVerma InfiniteVerma deleted the iss125 branch October 4, 2022 19:54
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.

Simplify makefiles
2 participants