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

[macOS] added curl download method to fetch-params.sh #2784

Merged
merged 2 commits into from Apr 14, 2018

Conversation

@kozyilmaz
Contributor

kozyilmaz commented Nov 30, 2017

added curl download method and flock workaround for macOS

Part of #2246.

@str4d

Could you please split this into two commits:

  • Add curl download method to fetch-params.sh
  • Add Darwin support to fetch-params.sh

@kozyilmaz kozyilmaz reopened this Dec 17, 2017

@kozyilmaz

This comment has been minimized.

Show comment
Hide comment
@kozyilmaz

kozyilmaz Dec 17, 2017

Contributor

@str4d split into two commits as requested

Contributor

kozyilmaz commented Dec 17, 2017

@str4d split into two commits as requested

@str4d str4d added this to 1.0.15: Misc in Release planning Dec 20, 2017

@str4d

str4d approved these changes Dec 20, 2017

utACK

@str4d str4d requested a review from bitcartel Jan 2, 2018

@str4d str4d added this to the 1.0.15 milestone Jan 2, 2018

@str4d str4d moved this from 1.0.15: Misc to 1.1.0: Misc in Release planning Feb 21, 2018

@str4d str4d modified the milestones: v1.0.15, v1.1.0 Feb 22, 2018

@str4d str4d requested a review from braddmiller Mar 10, 2018

PARAMS_DIR="$HOME/Library/Application Support/ZcashParams"
else
PARAMS_DIR="$HOME/.zcash-params"
fi

This comment has been minimized.

@braddmiller

braddmiller Mar 12, 2018

Contributor

Why is it necessary to change the directory where these params are installed?

@braddmiller

braddmiller Mar 12, 2018

Contributor

Why is it necessary to change the directory where these params are installed?

This comment has been minimized.

@str4d

str4d Mar 12, 2018

Contributor

The changed directory is in fact where zcashd has always looked for the parameters on MacOS; this script was just never downloading to the correct location, which wasn't noticed because we weren't testing it on Macs at the time.

@str4d

str4d Mar 12, 2018

Contributor

The changed directory is in fact where zcashd has always looked for the parameters on MacOS; this script was just never downloading to the correct location, which wasn't noticed because we weren't testing it on Macs at the time.

This comment has been minimized.

@str4d

str4d Mar 12, 2018

Contributor

Note that we still haven't seen a CI failure due to this yet, because parameter fetching happens after building in CI. We should merge this PR after the other MacOS PRs (fixing build issues) have been merged, so we see the failure that this PR fixes.

@str4d

str4d Mar 12, 2018

Contributor

Note that we still haven't seen a CI failure due to this yet, because parameter fetching happens after building in CI. We should merge this PR after the other MacOS PRs (fixing build issues) have been merged, so we see the failure that this PR fixes.

This comment has been minimized.

@braddmiller

braddmiller Mar 12, 2018

Contributor

Great thanks

@braddmiller

braddmiller Mar 12, 2018

Contributor

Great thanks

This comment has been minimized.

@kozyilmaz

kozyilmaz Mar 12, 2018

Contributor

This seems to be the params directory for macOS

return pathRet / "ZcashParams";

@kozyilmaz

kozyilmaz Mar 12, 2018

Contributor

This seems to be the params directory for macOS

return pathRet / "ZcashParams";

This comment has been minimized.

@bitcartel

bitcartel Mar 12, 2018

Contributor

With macOS, $HOME/Library/Application Support/... is where applications should by convention write their data. Btw, since macOS is still Unix under the hood, it could still be be written to $HOME/.zcash-params and work fine, but it would not be considered user friendly to place it there.

@bitcartel

bitcartel Mar 12, 2018

Contributor

With macOS, $HOME/Library/Application Support/... is where applications should by convention write their data. Btw, since macOS is still Unix under the hood, it could still be be written to $HOME/.zcash-params and work fine, but it would not be considered user friendly to place it there.

@braddmiller

This comment has been minimized.

Show comment
Hide comment
@braddmiller

braddmiller Mar 12, 2018

Contributor

This works as expected. I uninstalled wget and calling fetch-params.sh successfully defaulted to curl and began the download. See comment above for only question.

Contributor

braddmiller commented Mar 12, 2018

This works as expected. I uninstalled wget and calling fetch-params.sh successfully defaulted to curl and began the download. See comment above for only question.

@braddmiller

Tested ACK (Only tested that download works over curl)

@braddmiller

This comment has been minimized.

Show comment
Hide comment
@braddmiller

braddmiller Mar 22, 2018

Contributor

@zkbot try

Contributor

braddmiller commented Mar 22, 2018

@zkbot try

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Mar 22, 2018

Contributor

@braddmiller: 🔑 Insufficient privileges: and not in try users

Contributor

zkbot commented Mar 22, 2018

@braddmiller: 🔑 Insufficient privileges: and not in try users

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Mar 22, 2018

Contributor

A try of this PR won't show any effect in CI until #2820 is merged, because fetch-params.sh runs in CI after building.

Contributor

str4d commented Mar 22, 2018

A try of this PR won't show any effect in CI until #2820 is merged, because fetch-params.sh runs in CI after building.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Apr 4, 2018

Contributor

Bumping this to 1.1.1 even though #2820 was merged, because we still need to either adjust CI or fix linking in order for the build to pass (and for this change to be visible).

Contributor

str4d commented Apr 4, 2018

Bumping this to 1.1.1 even though #2820 was merged, because we still need to either adjust CI or fix linking in order for the build to pass (and for this change to be visible).

@str4d str4d moved this from 1.1.0: Misc to 1.1.1: Misc in Release planning Apr 4, 2018

@str4d str4d modified the milestones: v1.1.0, v1.1.1 Apr 4, 2018

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Apr 14, 2018

Contributor

We can now see the fetch-params.sh failure in CI!

@zkbot r+

Contributor

str4d commented Apr 14, 2018

We can now see the fetch-params.sh failure in CI!

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Apr 14, 2018

Contributor

📌 Commit 4c2120c has been approved by str4d

Contributor

zkbot commented Apr 14, 2018

📌 Commit 4c2120c has been approved by str4d

zkbot added a commit that referenced this pull request Apr 14, 2018

Auto merge of #2784 - kozyilmaz:fetch, r=str4d
[macOS] added curl download method to fetch-params.sh

added curl download method and flock workaround for macOS

Part of #2246.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Apr 14, 2018

Contributor

⌛️ Testing commit 4c2120c with merge ceb4559...

Contributor

zkbot commented Apr 14, 2018

⌛️ Testing commit 4c2120c with merge ceb4559...

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Apr 14, 2018

Contributor

☀️ Test successful - pr-merge
Approved by: str4d
Pushing ceb4559 to master...

Contributor

zkbot commented Apr 14, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing ceb4559 to master...

@zkbot zkbot merged commit 4c2120c into zcash:master Apr 14, 2018

1 check passed

homu Test successful
Details

@kozyilmaz kozyilmaz deleted the kozyilmaz:fetch branch Aug 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment