[tool] verify download URLs; docs for base URLs#94178
Conversation
yjbanov
commented
Nov 24, 2021
- When downloading artifacts validate the URLs and issue a warning if the URL is unexpected (or hard-fail in tests)
- Add dartdocs to base URL getters
1d46802 to
b4c09b3
Compare
There was a problem hiding this comment.
Is this manually maintained?
There was a problem hiding this comment.
In other words, if I add a new CIPD artifact to the tool, will it be present at https://storage.flutter-io.cn/flutter_infra_release/cipd? If not what is the process for getting it added? I would argue we should not allow further additions to this flow.
There was a problem hiding this comment.
I want to say yes, but I don't fully understand the question. Are the docs manually maintained? The code? Is there anything that can be automated?
There was a problem hiding this comment.
Ah, yes, it will be expected to be present at https://storage.flutter-io.cn/flutter_infra_release/cipd. I imagine @chenglu already has a process for identifying newly added artifacts and pushing them to the mirror. Whether we should be adding more CIPD artifacts is a little orthogonal. If they are added, they will be downloaded from this URL. If not, then it doesn't matter.
There was a problem hiding this comment.
I imagine @chenglu already has a process for identifying newly added artifacts and pushing them to the mirror.
Is this the case? Certainly they're not mirroring all of https://chrome-infra-packages.appspot.com/dl, right? If it's just the Flutter public ones, that might be feasible, but is that the case?
There was a problem hiding this comment.
Actually, now that I think about it, flutter precache --dry-run would be a useful option.
Agreed: #82383
IOW, you don't need all of CIPD, just the parts downloaded by the Flutter SDK, which can be enumerated using flutter precache.
This is possible, although I'm pretty sure that's not how the mirroring works today. I also think this would be difficult to do without flutter precache --dry-run, as it would involve parsing the flutter_tools source to determine the remote paths.
There was a problem hiding this comment.
A workaround is to point FLUTTER_STORAGE_BASE_URL to localhost and use a local server that prints every URL fetched by Flutter and responds with an empty file :)
There was a problem hiding this comment.
https://chrome-infra-packages.appspot.com/dl/flutter/web/canvaskit_bundle/+/1e8cK_8LOs0dz4lqd20LwTUYNqfu_4YL-dFG5yK1xXQC
-->
https://storage.flutter-io.cn/flutter_infra_release/cipd/flutter/web/canvaskit_bundle/+/1e8cK_8LOs0dz4lqd20LwTUYNqfu_4YL-dFG5yK1xXQC
This should be worked now.
Let me try to explain how the mirror works, which is essentially just a reverse host proxy that "whitelist" some of the gcs buckets like dart-archive/ ,download.flutter.io/ as well as flutter_infra_release/, so it can be accessed with the url storage.flutter-io.cn/dart-archive/ , storage.flutter-io.cn/download.flutter.io/, storage.flutter-io.cn/flutter_infra_release/, etc.
For the CIPD case, I pointed the path flutter_infra_release/cipd to chrome-infra-packages.appspot.com/dl, and the request will follow to get the redirected source (since the chrome-infra-packages.appspot.com/dl will 301 to https://storage.googleapis.com/chrome-infra-packages/ with a looong gcs credentials). The Tsinghua University (tuna mirror) and Shanghai Jiao Tong University (sjtug mirror) are partially dependent on the flutter-io.cn mirror, so the changes we make will take effect on both of them as well.
But my personal suggestion is that it would be better if these resources could be copied directly from the internal into gcs://flutter_infra_release/cipd when the build is done, thanks!
There was a problem hiding this comment.
But my personal suggestion is that it would be better if these resources could be copied directly from the internal into gcs://flutter_infra_release/cipd when the build is done, thanks!
This will happen when we start building CanvasKit from sources as part of the engine build (@hterkelsen may be able to provide an ETA). The reason it is a CIPD package today is because we consume it as a binary that's prebuilt by the Skia team for us. It's temporary.
There was a problem hiding this comment.
Hmm...We don't need to point out a specific country here. Also, corportate -> corporate.
| /// Some environments cannot reach the Google Cloud Storage buckets and CIPD due | |
| /// to firewall restrictions. A prominent example is China, where the whole | |
| /// country is behind a firewall, but various corportate environments may | |
| /// enforce this as well due to company-specific security policies. | |
| /// Some environments cannot reach the Google Cloud Storage buckets and CIPD due | |
| /// to firewall restrictions, various corporate environments may enforce this | |
| /// as well due to company-specific security policies. |
There was a problem hiding this comment.
Done. Heads up though, China is already mentioned 5 times in this file. If we don't want to mention it, we should probably clean it up.
There was a problem hiding this comment.
Haha, a clean up would be good.
b4c09b3 to
877a3f9
Compare
AlexV525
left a comment
There was a problem hiding this comment.
LGTM with a further cleanup. Tests seems can be resolved by rebase.
chenglu
left a comment
There was a problem hiding this comment.
LGTM, thanks again for doing this for CN Devs!
877a3f9 to
9001b52
Compare