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

feat(emulator): allowing to download and run emulator from URL #120

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Dec 3, 2021

@grdddj grdddj requested a review from tsusanka December 3, 2021 11:26
@grdddj grdddj linked an issue Dec 3, 2021 that may be closed by this pull request
Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome 🎉.

There is one thing I do not know: do we want to support generic URLs or are we fine with Gitlab links? In other words do we want to support downloads from anywhere?

Because if we limit it for Gitlab, we can simplify some code and also the UI. We would fill the artifacts names automatically. We could also request the job ID (1839236800) only.

Do we see use-cases where you would want to use some URL outside of Gitlab? cc @matejcik @mroz22

We could of course do both..


Also this is a great start and if we want to support branches easily we can do that by crafting a special URL. This URL https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/artifacts/master/download?job=core%20unix%20frozen%20debug%20build always leads to the latest core build on master. Replacing the master with any branch will do the same trick. But let's finish this PR first and worry about that then.

docs/controller.md Outdated Show resolved Hide resolved
docs/controller.md Outdated Show resolved Hide resolved
@matejcik
Copy link

matejcik commented Dec 6, 2021

The primary usecase for me would be to supply a local file for use.

This would cover the gitlab usecase too, we can fetch the artifact however we like and supply the local file to tenv.

Is there a usecase that actually wants tenv to download a specified URL?

@matejcik
Copy link

matejcik commented Dec 6, 2021

To be specific: although perhaps convenient in some situations, I never need to upload a new emulator while tenv is running. A well documented docker cp would work just fine, together with version "locally supplied" parallel to master.
-- that, or the ability to specify local path at runtime (but the URL thing would work with a file path too, maybe?)

@grdddj
Copy link
Contributor Author

grdddj commented Dec 6, 2021

Is there a usecase that actually wants tenv to download a specified URL?

This would allow the firmware CI (where the artifacts get created) and any tests using tenv (Suite, Connect) being independent (running on different pipelines, environments, etc.). Also for the manual testing it could be useful so that QA or anybody else can test the behavior of any emulator being built at Gitlab or anywhere else.

@matejcik
Copy link

matejcik commented Dec 6, 2021

This would allow the firmware CI (where the artifacts get created) and any tests using tenv (Suite, Connect) being independent (running on different pipelines, environments, etc.).

supplying a local file would work just as well -- the CI pipeline instances are not talking to the same docker instance running tenv, are they?

Also for the manual testing it could be useful so that QA or anybody else

Yes, this seems nice. In that case we can do what @tsusanka suggests and accept a link to the job instead of to the artifact.
I'm not sure how much it helps though. It's a UX nicety but other than that the code is essentially the same?

@grdddj
Copy link
Contributor Author

grdddj commented Dec 6, 2021

To be specific: although perhaps convenient in some situations, I never need to upload a new emulator while tenv is running. A well documented docker cp would work just fine, together with version "locally supplied" parallel to master. -- that, or the ability to specify local path at runtime (but the URL thing would work with a file path too, maybe?)

I did some prototype with docker cp some time ago in trezor/trezor-suite#4107 - after starting the tenv container we can copy any emulators into it and use them as 2-master - alternatively giving them some special name like 2-custom. We can theoretically do it in runtime as well, however probably only by pushing the emulator inside again by docker cp, I do not think docker container could get files from the local machine freely (but I would be glad to be mistaken)

@matejcik
Copy link

matejcik commented Dec 6, 2021

I see, I must have missed that. It looks like it would work fine for our CI usage. Specific name like custom would be a better solution than X-master (probably not even including version, because tenv can't know and verify that it's the right version), but that's a cosmetic thing.

@grdddj
Copy link
Contributor Author

grdddj commented Dec 6, 2021

the CI pipeline instances are not talking to the same docker instance running tenv, are they?

Each pipeline using tenv is getting the image from registry.gitlab.com/satoshilabs/trezor/trezor-user-env/trezor-user-env:latest, so I think they do not use the same instance, just the same base image.

@grdddj
Copy link
Contributor Author

grdddj commented Dec 6, 2021

It looks like it would work fine for our CI usage.

So we could wait after all the jobs in build CI stage are finished, take the core/build/unix/trezor-emu-core file, transfer it into the tenv as 2-custom and tell the Connect tests to use 2-custom tests?

Connect's main testing script, https://github.com/trezor/connect/blob/develop/tests/run.sh, already has the -f option to specify the emulator version, so we can supply that 2-custom there.

I will be trying to implement this pipeline as our CI stage in trezor/trezor-firmware#1972

We should probably test is against T1 emu as well, but it has lower priority (as it is not changing so much)

@matejcik
Copy link

matejcik commented Dec 6, 2021

not all, just the one that produces the file we need (so presumably "core unix debug")
otherwise, yes, exactly.

@tsusanka
Copy link
Contributor

tsusanka commented Dec 7, 2021

Sooo if I sum it up: we will do FW tests using simple cp as we'll require the build job using needs so the build will be present inside the container. This is nice and simple so 👍. @grdddj tackles this in trezor/trezor-firmware#1972 so let's continue the discussion there.


Now regarding this PR: could we still make use of this URL feature? Exactly as you say I can imagine some use cases for QA or Suite devs where you need to test some particular branch (like a Taproot one recently). It seems like a nice feature but it is true that we should not implement something until we properly understand the need. @mroz22 what do you think of this? Would you find it useful if you could set Gitlab URL in tenv and simply launch the emulator from such pipeline?

@tsusanka
Copy link
Contributor

Let's change the URL to https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1839236825/ and fill in the details

@grdddj grdddj force-pushed the grdddj/get_emulator_from_url branch from 2056e17 to 459583e Compare December 14, 2021 14:55
@grdddj
Copy link
Contributor Author

grdddj commented Dec 14, 2021

Let's change the URL to https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1839236825/ and fill in the details

Force-pushed after rebase on master and the above-mentioned change.

It is currently possible to input either Gitlab job link (https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1839236825/), just the job ID (1839236825) or the complete link, which makes sure other resources other that Gitlab CI could be used.

UX and UI is another unknown, but I believe the functionality is much more important here.

Copy link
Contributor

@tsusanka tsusanka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny thing, feel free to force-push then :). Did not test, but let's see how it works :).

</div>
<div>
<!-- TODO: could be prefilling the t1-or-t2-select based on some keywords in URL
(like "trezor-emu-core" fills T2, "trezor.elf" fills T1) -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the comment, I think the select box is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in the force-pushed commit (force-pushed twice, second already rebased on current master, even though there was no conflict)

@grdddj grdddj force-pushed the grdddj/get_emulator_from_url branch from 9112aff to 58df44e Compare December 14, 2021 17:35
@grdddj
Copy link
Contributor Author

grdddj commented Dec 14, 2021

One tiny thing, feel free to force-push then :). Did not test, but let's see how it works :).

Force-pushed after squashing everything

@grdddj grdddj force-pushed the grdddj/get_emulator_from_url branch from 58df44e to f458da2 Compare December 14, 2021 17:38
@tsusanka
Copy link
Contributor

I am getting a conflict now. Probably due to #126.

@grdddj grdddj force-pushed the grdddj/get_emulator_from_url branch from f458da2 to e8881e5 Compare December 14, 2021 18:16
@grdddj
Copy link
Contributor Author

grdddj commented Dec 14, 2021

I am getting a conflict now. Probably due to #126.

Correct, there was a need of rebase. Force-pushed now

@tsusanka tsusanka merged commit 4356c3a into master Dec 15, 2021
@tsusanka tsusanka deleted the grdddj/get_emulator_from_url branch December 15, 2021 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use custom firmware
3 participants