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

update typing-extensions dependency #572

Closed
prusnak opened this issue Feb 8, 2022 · 14 comments
Closed

update typing-extensions dependency #572

prusnak opened this issue Feb 8, 2022 · 14 comments

Comments

@prusnak
Copy link
Collaborator

prusnak commented Feb 8, 2022

In setup.py we have 'typing-extensions>=3.7,<4.0', but recently version 4.x has been released. We should test whether 4.x version works too and if it does we can relax the dependency to 'typing-extensions>=3.7,<5.0'

@Sjors
Copy link
Member

Sjors commented Jun 8, 2022

On macOS 12.4 with Python 3.9.13 via PyEnv, after doing poetry install I got a No module named 'typing_extensions' when calling ./hwi —version. But not sure if related.

@Sjors
Copy link
Member

Sjors commented Jun 8, 2022

Mmm, actually I think that error was because after a poetry install I also need a poetry shell in order for HWI to work, though the README suggests that's not necessary.

@k9ert
Copy link
Contributor

k9ert commented Feb 18, 2023

I've searched the code for usage of it and i got 7 hits, 5 of those are coming from hwilib/devices/trezorlib/.... That code comes imho from trezor-firmware which already uses 4.0.1 from typing extensions.
The other 2 are from docs/conf.py and from hwilib/_serialize.py.

However, HWI seem to use version 0.13.1 introduced Dec 21, 2021 but typing extensions 4.0.1 had been introduced in trezorfirmware on Feb 15, 2022 and released in version v0.13.1 (SIC!) on Jun 29, 2022 (Couldn't find any prior python-release after the Feb 15 2022).

Now, i'm a little bit surprised that the version 0.13.1 got introduced into HWI about half a year before it got released but apart from that, i'd think that it make the most sense before raising the typing_extensions dep to upgrade the hwilib/devices/trezor to at least the version ... [please help me out here].

Or did i miss something important?

@achow101
Copy link
Member

Now, i'm a little bit surprised that the version 0.13.1 got introduced into HWI about half a year before it got released but apart from that, i'd think that it make the most sense before raising the typing_extensions dep to upgrade the hwilib/devices/trezor to at least the version ... [please help me out here].

The version numbers don't actually match the commits we pull. We use whatever is in their git repo at that time, with some modifications.

@prusnak
Copy link
Collaborator Author

prusnak commented Feb 18, 2023

Or did i miss something important?

At Trezor we raise versions in git immediately after the release (that is when you release 0.13.0, you go and bump version to 0.13.1 in master). When Andrew pulled in changes from our git somewhere between the 0.13.0 and 0.13.1 release, he got the 0.13.1 version.

@k9ert
Copy link
Contributor

k9ert commented Feb 20, 2023

But would you agree that #666 doesn't make that much sense without upgrading the underlying code base?

@prusnak
Copy link
Collaborator Author

prusnak commented Feb 20, 2023

But would you agree that #666 doesn't make that much sense without upgrading the underlying code base?

Yes, I agree

@k9ert
Copy link
Contributor

k9ert commented Feb 21, 2023

But wouldn't it also make sense to carve out that folder and make it better portable? I mean, copying "versioned" code from one folder to the other is not what i'd consider as good engineering practices?
But there is probably a reason why it's done like that?

@prusnak
Copy link
Collaborator Author

prusnak commented Feb 21, 2023

I mean, copying "versioned" code from one folder to the other is not what i'd consider as good engineering practices?

I agree that ideally, trezorlib would be used as a dependency, not in-tree copy of sources.

The reason why it was done this way was that Andrew used custom modifications to be able to run trezorlib with Keepkey and other Trezor clones.

However, I think this should be revisited since the amount of custom modifications is now much much smaller and Python is great for monkey-patching, so you can still import the official trezorlib and monkey-patch your modifications on top of it.

@achow101
Copy link
Member

The reason why it was done this way was that Andrew used custom modifications to be able to run trezorlib with Keepkey and other Trezor clones.

It was also done that way to cut out all of the altcoin stuff.

@prusnak
Copy link
Collaborator Author

prusnak commented Feb 21, 2023

It was also done that way to cut out all of the altcoin stuff.

This has been also addressed and now altcoins dependencies are listed as extra_dependencies installable via pip install trezor[extra]. If you just do pip install trezor, no altcoin support stuff is installed.

achow101 added a commit that referenced this issue Feb 21, 2023
ad29646 ci: Bump protoc version (Andrew Chow)
08b5d93 trezor: Fix is_p2sh call (Andrew Chow)
d3cee84 poetry lock (Kim Neunert)
bda4e57 bump typing-extensions dependency (Kim Neunert)

Pull request description:

  Upgrade typing-extensions dependency ( #572)

  I chose not the most recent one but try to also be uptodate.
  ![image](https://user-images.githubusercontent.com/117085/219090741-c01d8b14-c15b-464f-9f7f-ff4b4fb20515.png)

Top commit has no ACKs.

Tree-SHA512: 91642a5401333bcf974366cd67acc161e4fae9509846fe9d89fbda6057e15c14d0536d4eaaf4d2ca97183a16947ec43fc3c4aec8f7006748a3b7b8cfe8649b57
@k9ert
Copy link
Contributor

k9ert commented Feb 27, 2023

So, @prusnak does it make sense that SatoshiLabs takes over the responsibility of maintaining its library integration into HWI? That way, you can make sure that updates are shipped much more often and the integration is much smoother and easier to maintain?
And the benefits for reaching a lot more trezor customers with this are (imho) also very clear, no?

@prusnak
Copy link
Collaborator Author

prusnak commented Feb 27, 2023

@k9ert We are already helping where we can with the integration. It seems to me that Andrew wants to have a more stable codebase instead of more frequent updates, which is why he decided to maintain the Trezor code in-tree. If that's not the case, I would be happy to get to a state where Trezor is used as an external library and HWI is simply using it instead of shipping in-tree copies of trezorlib.

@achow101
Copy link
Member

This was resolved by #666

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

Successfully merging a pull request may close this issue.

4 participants