Skip to content
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 install #31

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

add install #31

wants to merge 11 commits into from

Conversation

xgdgsc
Copy link

@xgdgsc xgdgsc commented Jun 19, 2020

Not sure if you think the include folder name is OK.

@syang0
Copy link
Member

syang0 commented Jun 19, 2020

Hello xgdgsc,

Your patch is still missing headers. Please add an integration test for your install and ensure it works before submitting another pull request. Your integration test should additionally be added as part of the GitHub workflow to ensure it runs upon the pull request as well.

Best Regards,
Stephen Yang

@syang0
Copy link
Member

syang0 commented Jun 23, 2020

Hey xgdgsc,

I'm not sure if you've finished the revisions, but looking at the changes, there appears to be two problems still.

  1. The integration test is failing because I think you need sudo to install the library to /usr/local/lib and
  2. Your integration tests don't properly test the installed library. More specifically, the last step builds the sample application by invoking make in the sample subdirectory. This build command will actually include the headers and library from the runtime directory rather than pulling it from the system folders.

Best Regards,
Stephen Yang

@syang0
Copy link
Member

syang0 commented Jul 1, 2020

Hi xgdgsc,

There is still one more problem with the patch. Your test application still does not completely test the installed headers. More specifically, it builds sample's main.o with the default rules, which would link with NanoLog/runtime/*.h headers rather than the system headers.

Best Regards,
Stephen Yang

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.

2 participants