Change TransactionBase into a Protocol

[lekcyjna123]

Here is a proposition of solution to problem found in #471. In TransactionBase we try to enforce closed set of types (TransactionOrMethod) in place where python provide us an open set (TransactionBase). This open set is a result of inheritance, where there is an assumption, that base class implementation should be common for all derived classes. We workarounded that by asserting if argument is a Method or a Transaction. But this workaround caused cyclic dependencies in imports after split of core.py.

Proposed solution use Protocol instead of inheritance to fully move type checking into static analysis. This change allow us to add type hint TransactionOrMethod to self in context function, which is not possible in plain base class (where type of self have to be superclass of its class type).

[lekcyjna123]

It looks like there is some incompatibility between CI and my local setup. Locally all tests passed. I suppose that this is because of the python 3.11 which I use locally.

[lekcyjna123]

From What's new in python 3.11:

The __init__() method of Protocol subclasses is now preserved. (Contributed by Adrian Garcia Badarasco in gh-88970.)

This probably explain, why tests on 3.10 fails.

[tilk]

This looks like a great reason to move to 3.11. Please do so in a separate PR though.

[lekcyjna123]

Yes. Update in this PR is for testing purposes. When all check pass, I will move them to separate MR. I am building now docker images with updated python. Do I need any additional permissions to upload docker images to ghcr.io or should it work without them?

[tilk]

We don't have a workflow which builds Docker images. And some permissions will probably be needed, but I don't really remember how this worked.

[lekcyjna123]

I have build images, but I don't have access to push them into kuznia-rdzeni namespaces. To my private namespace I am able to push.

I have found such instruction:
https://docs.github.com/en/packages/learn-github-packages/configuring-a-packages-access-control-and-visibility#about-setting-visibility-and-access-permissions-for-packages

[robtaylor]

Should just be a matter of declaring the right perms in the work flow file. Happy to help if needed.

…

On Sun, 8 Oct 2023 at 16:32, Marek Materzok ***@***.***> wrote: We don't have a workflow which builds Docker images. And some permissions will probably be needed, but I don't really remember how this worked. — Reply to this email directly, view it on GitHub <#472 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB4O4CJ6OIN23OTQ6IXWLDX6LBQDAVCNFSM6AAAAAA5XWMGG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJSGA3DQMBSHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[tilk]

@lekcyjna123: You got write permissions for the packages, please don't mess up stuff.

[tilk]

Why did you create a new Docker package?

[lekcyjna123]

amaranth-synth-ecp5 is renamed amaranth-synth this is because of the tag ecp5 used in the original one. I haven't seen possibility to update it in consistent way. But as I think about it now, then maybe instead of changing package base name I should change tag to ecp5-2023.10.09.

riscv-toolchain I uploaded into personal space, because I haven't had permissions to upload into kuznia-rdzeni space.

[lekcyjna123]

In the newest version I had to use flag --break-system-packages in workflows, which don't use python setup. So we should analyse in future how to remove that flag and properly use caches in the rest of our containers.

I see that all tests passed. Tomorrow I will create new PR with python update.

[tilk]

In the newest version I had to use flag --break-system-packages in workflows,

Looks smelly. Why did you use it? (You seem quick to hack things, but you rarely explain why.)

[Kristopher38]

So we should analyse in future how to remove that flag

python -m venv venv
source venv/bin/activate

:)

(This creates a virtual environment in directory venv and "activates" it, after that you can install the packages as if they were being installed system-wide, but they get installed into the venv directory instead, which you must source venv/bin/activate on any new terminal session if you want to use the packages installed there)

Also if you ever want to run python version not available on your distro but don't want to compile from source I recommend miniconda

Why did you use it?

I noticed that somewhere around half a year ago pip started requiring installing packages into virtualenvs instead of system-wide. If you really want to do so you can pass --break-system-packages option.

[Kristopher38]

This looks like a great reason to move to 3.11. Please do so in a separate PR though.

Afaik there was a bug on python 3.11 where signal width would get trimmed to 1 bit in vcd dumps. I was supposed to find a minimal example and submit an issue but forgot, not sure if that bug is still present though.

[tilk]

Afaik there was a bug on python 3.11 where signal width would get trimmed to 1 bit in vcd dumps. I was supposed to find a minimal example and submit an issue but forgot, not sure if that bug is still present though.

If it is, it's probably time to have it fixed.

[Kristopher38]

Afaik there was a bug on python 3.11 where signal width would get trimmed to 1 bit in vcd dumps. I was supposed to find a minimal example and submit an issue but forgot, not sure if that bug is still present though.

If it is, it's probably time to have it fixed.

This was fixed 5 months ago and we updated amaranth to a version which includes that fix, so it's no longer an issue. Full details here

[Kristopher38]

Looks sensible from my perspective, though I'm not an expert on python typing.