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

Add clone-norpm target #655

Closed
eloquence opened this issue Feb 4, 2021 · 5 comments · Fixed by #656
Closed

Add clone-norpm target #655

eloquence opened this issue Feb 4, 2021 · 5 comments · Fixed by #656

Comments

@eloquence
Copy link
Member

For working on dom0 tests, it's helpful to skip the RPM build step when cloning the repo given how long it takes. To avoid writing the same local hacks over and over again, I suggest we add a clone-norpm target or similar that does exactly that.

@emkll
Copy link
Contributor

emkll commented Feb 10, 2021

For background, a long series of issues describing the reasons we migrated towards RPMs for the dev environment, described in #538 .

It seems likely we are to repeat some of the errors from the past (e.g. RPM manifest issues), at the (increased) cost of maintaining two clone commands for the dev environment (and 4 distinct environments in all, which may be unnecessary).

@conorsch
Copy link
Contributor

How long is cloning taking? For me, it's 4m the first time, and 5s after that:

[user@dom0 securedrop-workstation]$ time make clone
Building RPM on sd-dev ...
Cloning code from sd-dev:/home/user/securedrop-workstation ...

real	3m55.946s
user	0m0.130s
sys	0m0.180s
[user@dom0 securedrop-workstation]$ time make clone
Building RPM on sd-dev ...
Cloning code from sd-dev:/home/user/securedrop-workstation ...

real	0m5.050s
user	0m0.140s
sys	0m0.145s

The very first time a build is ever run, it'll likely be longer, to create the image. Depending on the freshness of the local (i.e. local to sd-dev) docker volume cache, this can be a very snappy process. Perhaps we should add docs on preserving the docker volume cache inside sd-dev:

$ cat /rw/config/qubes-bind-dirs.d/50_user.conf 
binds+=( '/var/lib/docker' )

@eloquence
Copy link
Member Author

To clarify, what motivated this change for me is that making small changes to dom0 tests (without actually changing the installation state) took a look time. I'll test performance across runs w/ and w/o the caching changes you describe.

@eloquence
Copy link
Member Author

In my own testing without any modifications, the first run took 3 minutes, while subsequent ones took <10 seconds. My vote would still be in favor of adding the target to give devs the option to reliably skip that performance hit when working on changes that don't require an RPM, but I defer to @emkll and am happy to close #656 if you don't think it's needed.

@emkll
Copy link
Contributor

emkll commented Feb 10, 2021

If #656 is helpful to you @eloquence , given the size of the diff is fine to merge, we can always rename/remove if it becomes confusing.

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.

3 participants