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.

Sign up for GitHub

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

Jump to bottom

fix to makefile for OSX/Darwin #724

Merged
merged 1 commit into from
Apr 22, 2019
Merged

fix to makefile for OSX/Darwin #724

merged 1 commit into from
Apr 22, 2019

Conversation

mandric
Copy link
Contributor

@mandric mandric commented Apr 20, 2019

No description provided.

@codecov-io
Copy link

codecov-io commented Apr 20, 2019 •
edited
Loading

Codecov Report

Merging #724 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #724   +/-   ##
=======================================
  Coverage   85.32%   85.32%           
=======================================
  Files          64       64           
  Lines        3993     3993           
=======================================
  Hits         3407     3407           
  Misses        399      399           
  Partials      187      187

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64f154d...bafa6ad. Read the comment docs.

drasko
drasko previously approved these changes Apr 20, 2019
View reviewed changes
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

dborovcanin
dborovcanin reviewed Apr 20, 2019
View reviewed changes
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Please sign your commits.

@mandric
Copy link
Contributor Author

mandric commented Apr 22, 2019 •
edited
Loading

Sorry, added GPG signature to commit, rebased and pushed again. Also signed-off on my own commit.

manuio
manuio suggested changes Apr 22, 2019
View reviewed changes
Makefile Outdated
@@ -8,7 +8,7 @@ SERVICES = users things http normalizer ws coap lora influxdb-writer influxdb-re
DOCKERS = $(addprefix docker_,$(SERVICES))
DOCKERS_DEV = $(addprefix docker_dev_,$(SERVICES))
CGO_ENABLED ?= 0
GOOS ?= linux
GOOS ?= $(shell go env GOOS || echo linux)
Copy link
Contributor

Choose a reason for hiding this comment

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

GOOS ?= $(go env GOOS) should also work for linux

Can you please also add this ones:
GOARCH ?= $(go env GOARCH)
GOARM ?= $(go env GOARM)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mandric please use these simpler checks that @manuio proposed

Copy link
Contributor Author

@mandric mandric Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sure thing! But in my testing it needed shell keyword.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's working for us on Linux and OS X without this keyword. Why you need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the shell keyword I'm pretty sure the shell does not get invoked. It doesn't work for me without shell keyword, I just get an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mandric might be that Makefile demands this. Let me re-check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I checked - shell keyword is needed, so please just add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option that works on OSX here is just remove the GOOS ?= linux line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is where it was added: https://github.com/mainflux/mainflux/pull/196

@mandric mandric force-pushed the master branch 2 times, most recently from 84945ce to f78e877 Compare April 22, 2019 13:01
drasko
drasko approved these changes Apr 22, 2019
View reviewed changes
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit 08f2545 into absmach:master Apr 22, 2019
@drasko
Copy link
Contributor

drasko commented Apr 22, 2019

Meged! @mandric welcome to Mainflux contributors club ;).

@mandric
Copy link
Contributor Author

mandric commented Apr 22, 2019

Awesome, thank you!

davide83 pushed a commit to davide83/mainflux that referenced this pull request May 13, 2019
@mandric @davide83
fix to makefile for OSX/Darwin (absmach#724)
606c30c 
Signed-off-by: Milan Andric <[email protected]>
rugwirobaker pushed a commit to rugwirobaker/mainflux that referenced this pull request Jun 26, 2019
@mandric @rugwirobaker
fix to makefile for OSX/Darwin (absmach#724)
76ffc33 
Signed-off-by: Milan Andric <[email protected]>
manuio pushed a commit that referenced this pull request Oct 12, 2020
@mandric @drasko
fix to makefile for OSX/Darwin (#724)
071c8eb 
Signed-off-by: Milan Andric <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dborovcanin dborovcanin dborovcanin left review comments

@drasko drasko drasko approved these changes

@manuio manuio manuio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mandric @codecov-io @drasko @manuio @dborovcanin