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

Support a custom includes path #25

Merged
merged 8 commits into from
Sep 21, 2019
Merged

Conversation

codekoala
Copy link
Contributor

@codekoala codekoala commented Nov 15, 2017

This adds support for a custom includes path. This helps when you do not have permission to modify /usr/local/include, for example.

It relies on the standard -I flag, so something like make -I /tmp/custom/path and make -I/var/lib/whatever work.

@codekoala
Copy link
Contributor Author

@tj any thoughts on this? I have a project waiting on this feature for its build pipeline.

@tj
Copy link
Owner

tj commented Dec 8, 2017

hmm not something I need personally. I'm not a huge fan of replacing pkg-level globals from an API, but I think otherwise looks decent to me!

I kinda feel like using -I ruins any niceness that is provided by mmake but I'm not sure what your use-case is.

@codekoala
Copy link
Contributor Author

Thank you for the feedback!

I'm trying to use mmake in a build pipeline that does not provide write access to the build system's /usr/local/include directory. Using the -I ./include or whatever allows me to still pull in all of the dependencies using mmake's magic, but instead of writing those includes to /usr/local/include they're written to a location the build job has write access to.

// GetIncludePath allows the include path to be overridden.
func GetIncludePath(args []string) string {
// always reset the global IncludePath to the default
IncludePath = DefaultIncludePath
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this line necessary if it's defaulted above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. I know I had a reason for doing it this way, but that reason escapes me at this moment. I'll have to take a closer look at the whole diff when I get back to a computer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I added this line so the test results would be consistent. Without it, the expected outcome for later tests would not match. This being the case, I'll just reset IncludePath in the test instead.

@tj
Copy link
Owner

tj commented Dec 9, 2017

SGTM! just one question about the one line but otherwise I'll merge!

@codekoala
Copy link
Contributor Author

@tj this should be good to go now. Thanks!

@codekoala
Copy link
Contributor Author

@tj any other objections?

@zph zph mentioned this pull request Sep 16, 2019
9 tasks
Copy link
Collaborator

@zph zph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #34, I'm merging approved features to tidy up PR/issues.

Looks good @codekoala, thank you for the feature.

I'm considering submitting a PR later that makes this functionality available via an env variable MMAKE_INCLUDE. I know for my own environment I like to isolate where things are installed.... so default would stay the same but if MMAKE_INCLUDE is set then I could setup ~/.config/mmake/include as the storage location.

(Looks like my conflict resolution in GH interface failed on CI. Will pull down and re-do).

@zph
Copy link
Collaborator

zph commented Sep 21, 2019

Squashing and merging this and pushing a follow on commit to fix test reporting that changed. Thanks @codekoala!

@zph zph merged commit 5261f65 into tj:master Sep 21, 2019
@codekoala
Copy link
Contributor Author

codekoala commented Sep 21, 2019 via email

@zph
Copy link
Collaborator

zph commented Sep 21, 2019

👍 💃

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.

3 participants