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

Ability to override the default warehouse url base #7054

Merged
merged 2 commits into from May 17, 2016

Conversation

@laosb
Copy link
Collaborator

@laosb laosb commented May 14, 2016

Discussed about it at #6944. This PR gives you ability to override the default warehouse urlbase (warehouse.meteor.com) by setting METEOR_WAREHOUSE_URLBASE.

Use `METEOR_WAREHOUSE_URLBASE` to do so.
@laosb
Copy link
Collaborator Author

@laosb laosb commented May 15, 2016

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 17, 2016

Will anyone take a look? Just a simple change.

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 17, 2016

Added in History.md.

@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

Is there an actual mirror setup already? Or is this just in preparation for that?

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 17, 2016

Preparation. I've tried mirroring the warehouse, but had no luck in using it without the support of meteor tool.

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 17, 2016

Shouldn't fail with only a change in History.md. Rebuilding.

@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

You should still have luck using this change without this pull request by using your own local version of meteor-tool. Anyway, these are my general feelings on this change:

  • This still doesn't fix the problem of bootstrapping the initial install which seems like the biggest issue right now (packages are much smaller and seem to install fine). Do you disagree?
  • Maintaining a current and up-to-date mirror of warehouse.meteor.com seems like a complicated and potentially expensive task. The size of the warehouse alone must be relatively large. How do you plan on keeping it up to date?
  • This isn't a clean solution for end-users. They need to be told to set an environment variable based on their location? This isn't intuitive.
  • If #6944 gets resolved, then this is no longer an issue. There are CDNs who are successfully providing services in mainland China. If it's a hard task (which it probably is), this seems like a task that is best left to the CDN.
@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

I'll add on that there are definitely potential security risks with opening up the ability for third-party package mirrors that might not be properly secured, vetted, etc.

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 17, 2016

@abernix Most CDN providers don't provide nodes in mainland China, as China has it's own system of censoring websites. It's a hard work for a foreign company to do that.

For bootstrap bundles, it's much easier since we just need to replace the url in the script.

It's not the best solution, but can work for now. In Meteor China Community, everyday we get several persons complains about it, and doubts the magic of Meteor.

Yes, it's unsafe. What we can do is just add a warning -- but even most package managers didn't do that.

China is a totally different country when compares to others. For now, all package managers need a mirror, from Ubuntu dpkg, to npm. I don't think it can be solved by Meteor as a foreign company.

It's a hard task to maintain, but we'd like to try. Chinese CDN providers can do that as I know.

Truly. please give China developers a chance to try it out.

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 17, 2016

Pinging @yyx990803 as a Chinese might have some opinions about this.

@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

I'm still curious if most users having problems with bootstrapping or installing packages? And I'm not trying to avoid giving a China a chance, I promise! Just asking some questions. 😄 I don't think you've explained anywhere how this would work.

I'm aware of some of the unique challenges China presents. But running a mirror is a generally a very large task. I don't have any statistics on atmospherejs at all. Maybe you do? Space consumption? Daily traffic patterns? How many are accessing it from China right now? etc. Thankfully with Meteor moving toward NPM, the archive is (probably?) at it's peak and will likely decrease over time.

And still, I'm not sure why using an existing CDN wouldn't be a better solution – with geo-based DNS that would automatically use a China-based CDN instead of MaxCDN (no variables to change/worry about). There are CDNs there, even foreign – Yunjiasu (CloudFlare) (I don't think this was enabled previously, even though Meteor was using CloudFlare), Amazon CloudFront, uCloud, etc.

@abernix
Copy link
Member

@abernix abernix commented May 17, 2016

Also, there are changes to the install script itself right now that could be made to improve this –

  • curl could download to a local file (instead of piping directly to tar, as nice as that might be), which would allow:
    • retrying multiple times
    • auto-resume
    • checking MD5 and retrying
@zol zol merged commit 376df0d into devel May 17, 2016
4 checks passed
4 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@zol
Copy link
Contributor

@zol zol commented May 17, 2016

I've gone ahead and merged this as it's a simple uncontroversial change that may help others apart from @laosb

@n1mmy
Copy link
Member

@n1mmy n1mmy commented May 17, 2016

@zol we could also add a request parameter to install.meteor.com for CDN URL (like we do with release version). That way people in China or elsewhere can manually specify a URL to go in the install script. Between that and METEOR_WAREHOUSE_URL, that should be enough rope for anyone to establish a local mirror and instruct people how to use it. @laosb What do you think?

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 18, 2016

@n1mmy That would be fine.

@laosb laosb deleted the laosb-warehouse-urlbase-override branch May 18, 2016
@abernix
Copy link
Member

@abernix abernix commented May 18, 2016

Pretty disappointed that this was accepted without:

  • Discussion or a specific open issue addressing the problem and solutions (it was mentioned briefly in passing inside another issue)
  • At the very least, a console message indicating that you're using a non-standard download URL (this has potential to make debugging more complicated)
  • Ideas/instructions for how one would "establish a local mirror"
  • Trying to fix an issue that seemingly can be fixed in other ways (install script and download resiliency should be looked at first) – since this is happening with people in Germany using a datacenter in Germany too
@laosb
Copy link
Collaborator Author

@laosb laosb commented May 19, 2016

@abernix I had a simple conversation with @n1mmy in #6944. For the Point 2, yeah, I've considered doing so, but actually this env sounds like a for-debug hack and shouldn't be used by most developers.

When comes to instructions, I will try to do it after this PR get released and had someone use my testing cdn.

This is a quick fix for China users to be able to download packages again. Better solutions take time to do.

@laosb
Copy link
Collaborator Author

@laosb laosb commented May 19, 2016

Really hope can have this released soon.

@abernix
Copy link
Member

@abernix abernix commented May 19, 2016

@laosb I think you know my feelings on this already (as we already discussed in chat):

  1. This commit (by itself) is harmless. In fact, it's not even usable. This is one small part of a large, undesigned feature.
  2. It's really a matter of the process of adding a feature. A comment about a large feature (mirror support) inside another issue is not enough for a PR. See feature requests. You are a contributor too and we are supposed to work together to make sure features are well-designed before they become PRs.
  3. There is no working mirror right now that you can test this with.
  4. If there was a working mirror, you could test this right now without this commit.
  5. Building and maintaining a functioning, secure, syncing warehouse mirror is unlikely to be more of a "quick fix" than changing ~20 lines of code and trying that first.
  6. You asked for a review (multiple times in multiple places). This was my review.
  7. Again, as I told you directly, I'm happy to help design this with you and think accessibility is very important! 😄
@mitar
Copy link
Collaborator

@mitar mitar commented May 31, 2016

Oh, this is a great change. In the past I had to provide my own infrastructure for Meteor (http://meteor.peerlibrary.org/) and such hooks become really useful in such cases.

I think @awatson1978 might be interested in this as well.

But it would be great to now be able to create a mirror as well. It might be enough to just wrap my package with a hook which downloads files for all packages in a collection.

@mitar
Copy link
Collaborator

@mitar mitar commented May 31, 2016

So the rest can probably be provided by the community.

I also like the idea of the request parameter to install.meteor.com.

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Jun 16, 2016

Hrm... Somehow in the entire process of suggesting, writing, reviewing, merging, and shipping this patch, none of us (myself included!) read the comment at the top of the file:

This file is used to access the "warehouse" of pre-0.9.0 releases. This code is now legacy, but we keep it around so that you can still use the same meteor entry point to run pre-0.9.0 and post-0.9.0 releases, for now.

Oops! So this env variable doesn't actually let you control where modern meteor downloads packages from. In modern meteor the URL comes from the server and does not have a client coded base url. This happens here https://github.com/meteor/meteor/blob/devel/tools/packaging/tropohouse.js#L300 . If we want to provide client tweak-ability, of download URL, thats where we'd have to hook in.

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Jun 16, 2016

@benjamn Maybe it's time to remove pre 1.0 back compat? Incidents like this are the cost =(

@mitar
Copy link
Collaborator

@mitar mitar commented Jun 16, 2016

As somebody still running Meteor 0.8 I would vote not to remove. :-)

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Jun 16, 2016

0.8?!?! Wow... =)

@mitar
Copy link
Collaborator

@mitar mitar commented Jun 16, 2016

0.8.3, to be precise. :-) http://meteor.peerlibrary.org/

@mitar
Copy link
Collaborator

@mitar mitar commented Jun 16, 2016

(BTW, I am not sure if this particular change would impact me, but I just wanted to provide one datapoint about old Meteor versions.)

@laosb
Copy link
Collaborator Author

@laosb laosb commented Jun 17, 2016

No wonder why it doesn't cause any data usage on the test mirror!

@laosb
Copy link
Collaborator Author

@laosb laosb commented Jun 17, 2016

Actually I have looked around tropohouse.js before the suggestion from @n1mmy and noticed that comment. I'm not quite familiar with meteor-tool so I believe it should be right. What a terrible thing!

I'm not sure how to make this happen on modern Meteor tools then.

@laosb
Copy link
Collaborator Author

@laosb laosb commented Jun 17, 2016

Is buildRecord.build.url a full url that we can do a regex replace?

@n1mmy
Copy link
Member

@n1mmy n1mmy commented Jun 17, 2016

I believe so, yes. But then, I thought warehouse.js was the right spot to edit, too =)

@laosb
Copy link
Collaborator Author

@laosb laosb commented Jul 9, 2016

@n1mmy how does the url look like, https://warehouse.meteor.com/xxxx/xxx/xx so we can simply replace https://warehouse.meteor.com/ with METEOR_WAREHOUSE_BASEURL?

And use http instead of https to download won't fail, I guess?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.