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

chore(Makefile): detect and default to make go-install on darwin vs install everywhere else #3197

Merged
merged 4 commits into from
Mar 13, 2024

Conversation

ramin
Copy link
Contributor

@ramin ramin commented Feb 20, 2024

refs #3169

Suggestion we add this little detect and switch. One issue i see with attempting to detect GOBIN is that it doesn't HAVE to be set (ie: there can be a default). This i would think is a good quick compromise to un-confuse @rootulp and (potentially) other devs and make this behavior more delightful most of the time. IF not, we should close this and the other issue.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5e1895c) 51.81% compared to head (0f6beb7) 51.49%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3197      +/-   ##
==========================================
- Coverage   51.81%   51.49%   -0.32%     
==========================================
  Files         178      178              
  Lines       11286    11286              
==========================================
- Hits         5848     5812      -36     
- Misses       4930     4968      +38     
+ Partials      508      506       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramin ramin added the kind:misc Attached to miscellaneous PRs label Feb 20, 2024
Makefile Outdated Show resolved Hide resolved
@ramin ramin requested a review from Wondertan February 21, 2024 13:51
walldiss
walldiss previously approved these changes Mar 4, 2024
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM. The suggestion I made is regarding Makefile comments

Makefile Outdated Show resolved Hide resolved
@jcstein
Copy link
Member

jcstein commented Mar 4, 2024

for docs, should i just use default for ubuntu/mac as make install? after this PR is in the release on the networks?

Co-authored-by: Rootul P <rootulp@gmail.com>
@ramin ramin requested review from rootulp and walldiss March 6, 2024 09:56
@ramin ramin merged commit b0a724e into main Mar 13, 2024
27 checks passed
@ramin ramin deleted the chore/ramin/makefile-install branch March 13, 2024 11:01
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Apr 3, 2024
…nstall everywhere else (celestiaorg#3197)

<!--
Thank you for submitting a pull request!

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

Please ensure you've addressed or included references to any related
issues.

Tips:
- Use keywords like "closes" or "fixes" followed by an issue number to
automatically close related issues when the PR is merged (e.g., "closes
#123" or "fixes #123").
- Describe the changes made in the PR.
- Ensure the PR has one of the required tags (kind:fix, kind:misc,
kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci,
kind:chore, kind:testing)

-->

refs celestiaorg#3169

Suggestion we add this little detect and switch. One issue i see with
attempting to detect GOBIN is that it doesn't HAVE to be set (ie: there
can be a default). This i would think is a good quick compromise to
un-confuse @rootulp and (potentially) other devs and make this behavior
more delightful most of the time. IF not, we should close this and the
other issue.

---------

Co-authored-by: Rootul P <rootulp@gmail.com>
renaynay pushed a commit to renaynay/celestia-node that referenced this pull request Apr 3, 2024
…nstall everywhere else (celestiaorg#3197)

<!--
Thank you for submitting a pull request!

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

Please ensure you've addressed or included references to any related
issues.

Tips:
- Use keywords like "closes" or "fixes" followed by an issue number to
automatically close related issues when the PR is merged (e.g., "closes
#123" or "fixes #123").
- Describe the changes made in the PR.
- Ensure the PR has one of the required tags (kind:fix, kind:misc,
kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci,
kind:chore, kind:testing)

-->

refs celestiaorg#3169

Suggestion we add this little detect and switch. One issue i see with
attempting to detect GOBIN is that it doesn't HAVE to be set (ie: there
can be a default). This i would think is a good quick compromise to
un-confuse @rootulp and (potentially) other devs and make this behavior
more delightful most of the time. IF not, we should close this and the
other issue.

---------

Co-authored-by: Rootul P <rootulp@gmail.com>
Wondertan pushed a commit that referenced this pull request May 8, 2024
…only (#3340)

refs #3169 && #3197

Instead of make install, I was using make install-global because my path ordered the go/bin last. Took me a bit of time to clarify why the -t option wasn't working, so I had just removed it locally.. Anyway, figured I'd put this up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:misc Attached to miscellaneous PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants