Switch HTTP implementation to reqwest
#124
Conversation
The prior HTTP implementation was `hyper`, and provided a low level of abstraction. This low level required me to implement redirect and proxy support. I omitted proxy support. Now, we use `reqwest` to handle HTTP. This allows us to delete our custom (but not special) redirect support and pick up proxy support.
duplicate packagesbefore11 after11 It's weird that the same number of dupes are present, but they're not the same 😕 Checked with |
|
Surprisingly large change, I think you're doing a small re-factoring to some bits on the go? Question - how to test this? Wonder if the README.md could be expanded to give some advice how to run this locally (as part of the |
|
@JanneKiiskila Thanks for the feedback. Responses with quoted questions below.
Yeah, I compulsively run rustfmt, and it seems to have decided to reformat a tad.
I moved the vidx downloading code to be members on the
That's a good point. I'll write it here and then we can add the instructions to the readme. To build cmsis-pack-manager locally, Install a stable rust compiler. See https://rustup.rs/ for details. After installing the rust toolchain and downloading a stable compiler with For testing purposes, there is a minimal CLI written in Rust within the rust workspace |
|
@JanneKiiskila I pushed up a more detailed version of the build instructions. View them here: https://github.com/ARMmbed/cmsis-pack-manager/blob/596c117e70b80a04e05ba1ccd8b284b236f5d960/README.md. I also need to figure out what's going wrong with travis CI. |
|
Fwiw, I'm also having issues with Travis-CI with pyOCD. It does seem to run the tests but the results are not making it to GitHub. Haven't had time to look into it, so I'm very curious if you find anything. |
|
@theotherjimmy No I hadn't seen that, thanks for the link! |
|
@theotherjimmy Unfortunately the fix described in that link doesn't apply directly. But it does hint that the problem seems to be related to Travis-CI transitioning to GitHub Apps. Basically, we need to migrate these repos to using Travis via Apps, then make sure to follow the instructions in that link. |
|
Trial run shows we still have some issues, but I have no clear explanation/understanding why. Starting point - located in Annoyingly, it's exactly (still) one of those critical STM packages that is failing (with unexpected end of file). :-/ |
|
Much to my annoyance, So, there is some secret sauce |
|
The Active-Semi case is also weird, |
|
One question - in this case, where are the files downloaded? The current folder seems to be empty. |
|
@JanneKiiskila They should be in |
|
@JanneKiiskila As for the issues, I think they're TLS cypher suite mismatches. You could try applying the diff in #121 to see if that's the case. FYI, this cannot be packaged for linux with OpenSSL as the backend for TLS as it requires an ancient bug-ful version to be a manylunx1 binary. |
|
Indeed, that does resolve the issue. Question is - what are our options here? This needs to get fixed, we have some deadlines to meet... (Unless we buy time and actually update that one package manually). |
We have come rust compiler warnings:
```
warning: trait objects without an explicit `dyn` are deprecated
--> pdsc/src/component.rs:225:17
|
225 | ) -> Result<Box<Iterator<Item = ComponentBuilder>>, Error> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Iterator<Item = ComponentBuilder>`
|
= note: `#[warn(bare_trait_objects)]` on by default
Compiling reqwest v0.9.22
Compiling cmsis-update v0.1.0 (/home/jankii01/mbed/cmsis-pack-manager/rust/cmsis-update)
warning: trait objects without an explicit `dyn` are deprecated
--> cmsis-update/src/download.rs:153:14
|
153 | ) -> Box<Future<Item = (), Error = Error> + 'a> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Future<Item = (), Error = Error> + 'a`
|
= note: `#[warn(bare_trait_objects)]` on by default
warning: trait objects without an explicit `dyn` are deprecated
--> cmsis-update/src/download.rs:188:14
|
188 | ) -> Box<Stream<Item = PathBuf, Error = Error> + 'a>
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Stream<Item = PathBuf, Error = Error> + 'a`
```
The compiler is pretty cool, as the warning it gives already tells you
the real fix, too.
|
Other question - how can I get this to be in place for the Python tooling, with this version I could already actually do the required cmsis-pack update. Better yet would be if I could update the information from that one particular package ONLY to the I think just doing |
Updating of all CMSIS packs that running ``` python project.py -m STM32F7xx --update-packs ``` brings, while using a customer version of cmsis-pack-manager. (Essentially using the reqwest version from theOtherJimmy with the TLS fix mentioned in the comments). See: - pyocd/cmsis-pack-manager#121 - pyocd/cmsis-pack-manager#124 for more details.
|
@JanneKiiskila Yes would could do For clarification: pip is happy to create a |
Fix trait objects without explicit `dyn`.
|
Fwiw, there is a new |
|
Thanks for the link. We still don't have |
|
|
| DownloadContext { | ||
| pub fn new(config: &'a Conf, prog: Prog, log: &'a Logger) -> Result<Self, Error> { | ||
| let client = ClientBuilder::new() | ||
| .use_rustls_tls() |
There was a problem hiding this comment.
This part was having some issues still, what is the route we can consider going forwards to avoid the issue? I still had to tweak this part out to get it working (i.e. apply your patch).
There was a problem hiding this comment.
@JanneKiiskila Removing that line forces linux users to use Openssl instead of Rustls. This prevents the package from being a "manylinux" wheel, and you're only allowed to upload "manylinux" wheels to pypi. My hands are tied, I can't use Openssl here.
There was a problem hiding this comment.
Oh, that is a problem. :-/ Question is then can we file an issue to the RustLS and have those guys fixed the underlying TLS handshake/cipher issue?
There was a problem hiding this comment.
That really depends on the cypher suite is question. They have no plans to add support for known-broken cyphers.
There was a problem hiding this comment.
KEIL or AWS should not use broken cipher suites. The chosen cipher suite is selected with a negotiation, right? If the client proposes a good, working cipher suite - surely the back-end should use it?
There was a problem hiding this comment.
I agree, they should not use broken cypher suites. also, I think the hosting is Microsoft related, at least that's what I recall. It's also really frustrating to debug this issue, as the server in question simply terminates the connection instead of giving us any information. We might have negotiated for a cypher suite that segfaults for all we know.
There was a problem hiding this comment.
Seems we're getting more failures... :-(
(cmsis) jankii01@ubuntu:~/mbed/mbed-bootloader/mbed-os/tools$ python project.py --update-packs
Nov 12 16:04:06.756 ERRO download of "http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC5528_DFP.pdsc" failed: http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC5528_DFP.pdsc: connection closed before message completed
Nov 12 16:04:06.784 ERRO download of "http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC5526_DFP.pdsc" failed: http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC5526_DFP.pdsc: connection closed before message completed
Nov 12 16:04:06.807 ERRO download of "http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC55S28_DFP.pdsc" failed: http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC55S28_DFP.pdsc: connection closed before message completed
Nov 12 16:04:06.996 ERRO download of "http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC55S26_DFP.pdsc" failed: https://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC55S26_DFP.pdsc: connection closed before message completed
http://www.amiccom.com.tw/download/Amiccom_DeviceFamilyPack/Amiccom.SoC_DFP.pdsc: error trying to connect: Connection refused (os error 111)
Nov 12 16:07:58.866 ERRO download of "http://www.mindmotion.com.cn/Download/MDK_KEIL/MindMotion.MM32L3xx_DFP.1.0.6.pack" failed: http://www.mindmotion.com.cn/Download/MDK_KEIL/MindMotion.MM32L3xx_DFP.1.0.6.pack: error trying to connect: Connection refused (os error 111)
Nov 12 16:08:22.251 ERRO download of "http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC844_DFP.12.0.0.pack" failed: http://mcuxpresso.nxp.com/cmsis_pack/repo/NXP.LPC844_DFP.12.0.0.pack: connection closed before message completed
Nov 12 16:10:16.694 ERRO download of "http://www.mindmotion.com.cn/Download/MDK_KEIL/MindMotion.MM32F031_DFP.1.3.7.pack" failed: http://www.mindmotion.com.cn/Download/MDK_KEIL/MindMotion.MM32F031_DFP.1.3.7.pack: connection error: Connection reset by peer (os error 104)
The prior HTTP implementation was
hyper, and provided a low levelof abstraction. This low level required me to implement redirect and
proxy support. I omitted proxy support.
Now, we use
reqwestto handle HTTP. This allows us to delete ourcustom (but not special) redirect support and pick up proxy support.
Note: This contains format changes and a minor refactor (move
vidx/pidx-related code to methods on
DownloadContext)