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

Integrate aur-fetch and add diff support #37

Closed
wants to merge 1 commit into from

Conversation

Morganamilo
Copy link
Collaborator

@Morganamilo Morganamilo commented May 25, 2019

Plus reorder the directory structure

Now instead of the cache layout being <cache>/<pkgname>/<subdir> it is
now <cache>/<subdir>/<pkgname>.

E.g.

/home/morganamilo/.cache/rua
├── aur.tmp
│   ├── clone
│   └── diff
├── build
└── checked_tars

@Morganamilo
Copy link
Collaborator Author

P.S jailbuild seems totally broken. Even before I touched anything, so no idea what to do about that.

@vn971
Copy link
Owner

vn971 commented May 31, 2019

@Morganamilo in what way do you think jailbuild is broken? It certainly works for me. Maybe I got the documentation part wrong?

[19:23:43 vasya@vn971 ~] rua jailbuild --help
...
USAGE:
rua jailbuild [FLAGS]
...
ARGS:
Target directory

So you should use it like rua jailbuild /path/to/directory_containing_PKGBUILD/

@Morganamilo
Copy link
Collaborator Author

So you should use it like rua jailbuild /path/to/directory_containing_PKGBUILD/

Ah, I assumed it downloaded the pkgbuild.

@vn971
Copy link
Owner

vn971 commented May 31, 2019

Looking more at the MR, I think the caller (RUA) should be responsible for overall operation instead of callee library (aur-fetch). What it means exactly:

  • choosing directories. For example, placing diff/clones in ~/.cache/rua or in ~/.config/rua?
  • deciding what to store, e.g. store diffs or git repos or raw files?
  • should we apply patches, or do rebases, or do merges? Should we show diff between current dirty directory and previous accepted state, or between current dirty tree and current upstream, or between accepted state and upstream, or all of that?

The current library aur-fetch is very opinionated on a lot of those questions... I'm not sure how convenient it would be to try split this logic into a library that would (potentially) be reused by other AUR clients.

It would be very interesting for me, however, to try discuss the questions above, and then see what can we copy or reuse from current aur-fetch. My personal preferences:

  1. yes, small user-verified things should be placed in ~/.config, but large build artifacts should be in ~/.cache.
  2. I'm really not sure. I guess storing the git repo sounds appealing. But there should definitely be a way to show various kinds of diffs and propose various kinds of actions automatically. Not necessarily implemented right away, but at least possible.
  3. I would force user to do whatever they want if they patched the thing, otherwise proceed anyhow (all approaches will basically give same results if user didn't touch anything).

@vn971
Copy link
Owner

vn971 commented May 31, 2019

By the way, a side note. Technically, the build process may create stuff like git hooks, which would in the prososed setup be executed by RUA unjailed. This is pretty insane though, not sure we should even try to protect against that. But if we copy the git thing to ~/.cache before each build, we get it for free.

@Morganamilo
Copy link
Collaborator Author

choosing directories. For example, placing diff/clones in ~/.cache/rua or in ~/.config/rua?

Where it downloads files to is totally configurable.

deciding what to store, e.g. store diffs or git repos or raw files?

You can choose to store diffs or not store them. By raw files I assume you mean downloading tarballs? Downloading via git is faster in every way, so there's no advantage to it.

should we apply patches, or do rebases, or do merges? Should we show diff between current dirty directory and previous accepted state, or between current dirty tree and current upstream, or between accepted state and upstream, or all of that?

The current setup allows user's to edit pkgbuilds. If you just diffed HEAD with origin then you would lose local changes because origin does not have your changes. That's why it merges with upstream and gives you a diff of that.

@vn971
Copy link
Owner

vn971 commented May 31, 2019

Where it downloads files to is totally configurable.

Ah indeed, since the library doesn't touch building part, it only has one directory to configure, and it is enough. I've misread that, sorry. I'll get back to other points a bit later as well.

@Morganamilo
Copy link
Collaborator Author

Any new opinions on this?

@vn971
Copy link
Owner

vn971 commented Jun 25, 2019

@Morganamilo nothing new, but the old considerations still hold in my mind. E.g. I want building in ~/.cache but having cloned PKGBUILD repos in ~/.config, allow users to easily view full diffs without having go to git manually (upstream diff, diff old to local, diff new to local). Also, if git workflow would be required from user, check that the current branch has upstream merged in.

@vn971
Copy link
Owner

vn971 commented Jun 25, 2019

If you'd like to make the changes, I'd be happy to merge of course. Otherwise I planned to make appropriate changes and then merge.

Copy link
Owner

@vn971 vn971 left a comment

Choose a reason for hiding this comment

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

See PR discussion.

@Morganamilo
Copy link
Collaborator Author

I want building in ~/.cache but having cloned PKGBUILD repos in ~/.config

Is rua currently like this? I've not really payed attention. In fact if ruais currently like this then it's likely still like this and I just assumed otherwise.

allow users to easily view full diffs without having go to git manually

Not sure what you mean here. The user does not have to use git manually. Sure git is called internally by rua, but that's just because these are git repos and git provides a very easy way to generate diffs.

Also, aur-fetch does provide a diff_to_file function. Sue it still uses git to generate them, but you are then free to open the diffs in a file browser or whatever else.

(upstream diff, diff old to local, diff new to local)

What is old to local? Implemented here is local to upstream, which is the only thing I see that matters.

@vn971
Copy link
Owner

vn971 commented Jun 25, 2019

Is rua currently like this? I've not really payed attention. In fact if ruais currently like this then it's likely still like this and I just assumed otherwise.

It's not really. I'd just strongly like to see it this way before we have diff-ing and stuff. Because otherwise, people will start to rely on the diffs, and they will be broken if we change dir paths all of a sudden. Think of it as some kind of stabilization point.

Not sure what you mean here. The user does not have to use git manually.
What is old to local? Implemented here is local to upstream

I mean showing all kind of diffs, "old" means previously accepted. So:

  • local to previous accepted
  • local to upstream
  • previously accepted to upstream

@Morganamilo
Copy link
Collaborator Author

In my model, local is the same as accepted. So it only really makes sense to diff against upstream.

Does that sound reasonable to you?

@vn971
Copy link
Owner

vn971 commented Jun 25, 2019

I guess we can start with only one kind of diff. But other types of diffs need to be implementable by design in future.

Some other things that I remembered as well:

  • I'm not sure if there's value to having aur.tmp/clone and aur.tmp/diff. This grouping probably doesn't have any special meaning, so I'd flatten it.
  • I'd prefer to copy things from ~/.config to ~/.cache before build. So that you could wipe or not backup ~/.cache without losing anything important.

Plus reorder the directory structure

Pkgbuilds are also initially cloned into .config, before being coppied
to .cache for building
Rename aur.tmp to clone seems as downloades are now cached
Now instead of the cache layout being  <cache>/<pkgname>/<subdir> it is
now <cache>/<subdir>/<pkgname>.
@Morganamilo
Copy link
Collaborator Author

Handled those issues as well as adding coloured diffs. Still confused over the other kinds of diffs.

@vn971
Copy link
Owner

vn971 commented Jul 8, 2019

Sorry, had a trip to Dusseldorf and some other things, didn't have time to finish review yet. I'll try to do it in the following week.

@vn971
Copy link
Owner

vn971 commented Jul 17, 2019

// I still want to return to this, just was somewhat busy with other things, unfortunately.

@vn971
Copy link
Owner

vn971 commented Aug 5, 2019

Sorry for the very-very long review times. I've reviewed the code & tested it now. I tried to build a package now, but the current code just erases the local changes if I make any, not being persisted in ~/.config. Also, changes during rua install XXX are right now done directly in ~/.config/rua/clone, which feels wrong as you could imagine that clone means directories with cloned packages, the ones not having local changes.

I still don't think that abstraction of aur-fetch into a separate library makes a lot of sense, but I could live with that.

One other thing that makes sense I think is to try model the behavior without coding first, and once we inspect how it works and agree on something, transform it to code. Otherwise it's a bit sad to spend energy on something that can potentially be rewritten in the future anyway. Anyway, the remarks on the current code are above.

@vn971
Copy link
Owner

vn971 commented Aug 10, 2019

OK, so I'm thinking of implementing this. The current idea is the following:

  • when a package is fetched the first time, git clone it with the remote name being upstream. This will be the remote version of the package, as kept on ArchLinux servers.

  • when reviewing changes or letting the user to make changes, assume that

    • the current local changes is what the user has reviewed and accepted
    • the last commit in upstream/master is what the uptstream is on ArchLinux
    • the last common parent commit between current dirty directory and upstream/master is the previous build that the user has accepted
  • before build, check that HEAD of upstream/master is parent of current commit. Only continue when it's true, making sure the user has merged remote changes. In case of no local changes, allow of course to just fast-forward the current state to upstream/master and allow not interacting with git directly but just say "OK, I accept the changes". The latter would mean that the user reviews diff between master and upstream/master and accepts it.

  • when building, copy all files to ~/.cache/...` and never reuse git information from there.

  • if a package is needed to be built but upstream/master is alreagy merged in, ask the user what to do. TODO: or do something different? Check if artifacts are built as well and silently skip?

  • when a package is upgraded, fetch the upstream remote and proceed with diff.

That's more or less it I guess. Everything should work this way right? Local changes and upstream preserved, git knowledge can be re-used for easier merges, you can see full history of your changes and upstream + see all kinds of diffs. You can wipe ~/.cache and not lose anything important.

@vn971
Copy link
Owner

vn971 commented Aug 13, 2019

I decided to indeed make my attept at fixing it as well, since there are so many requirements and I'm not sure how they all feel on the other side. One other change that I want to make along the way is to fetch package information using raur instead of reading .SRCINFO locally. This allows ignoring local changes that the user has made, e.g. different commit and dirty tree in user-s ~/.config/rua/pkg/....

@vn971
Copy link
Owner

vn971 commented Aug 13, 2019

I encountered one problem with the current plan: what to do with package installation for which latest changes were already reviewed and accepted?

Still wipe ~/.cache/... and rebuild? See if actual targets (*.pkg.tar.xz) are built and wipe everything if not? Drop into a shell? Drop into a jailed shell? Present some of these choices to user? I would of course also like to have a minimalistic idea/solution that doesn't require too many edge cases.. Tricky situation, overall. I'll think of it more, any ideas are generally welcome.

vn971 pushed a commit that referenced this pull request Aug 16, 2019
* stop ever relying on current dir, current environment etc. This opens possibility for future concurrent workflow.

* add logging

* stop enforcing of target/ being the PKGDEST.
Use more flexible by-file tar review.

* resolve dependencies via RAUR library instead of relying on .SRCINFO-s

* add a separate `shellcheck` command for RUA, with built-in handling of PKGBUILD variables

* move all `include_bytes!` macros to one place

fixes GH-1, fixes GH-41, replaces GH-37, supersedes GH-42.
@vn971
Copy link
Owner

vn971 commented Aug 23, 2019

Closing in favor of the new merge requests that bring in shellcheck action, upgrade, and add support for the "diff" features with persistence support.

If something would be valuable to have as a library, and the functionality can be moved to a separate crate, it can be OK. On the other hand, there's not much to move, I only see this file as a good abstraction: https://github.com/vn971/rua/blob/21ca0de5c31280d664eacb6bec6512ebf8e6b26b/src/git_utils.rs
It's possible of course to move parts of "tar_check" functions to a library as well, but I'd prefer to do it only knowing there's a practical reason for that. If that is the case, a new correspinding issue can be opened.

@vn971 vn971 closed this Aug 23, 2019
@vn971
Copy link
Owner

vn971 commented Aug 23, 2019

I'm not sure if that would feel encouraging or not, but note that I did embrace a lot of the proposed concepts in the end. The directory layout is now "grouped" around the stage first (as in the first message in this PR). I ended up using git to store user-s review state, with git merge being the action to bring in upstream changes. I think it's a very neat representation indeed, though I'm not sure if the specifics of using git are the same now to what it was proposed originally.

Basically, I simplified git-related commands to only include git init, git merge upstream/master and git diff upstream/master (no git reset --hard for example), and also did a complete revamp of how packages are built.

Hope you understand why it was needed, and thanks for proposing those awesome ideas.

@Morganamilo
Copy link
Collaborator Author

Morganamilo commented Oct 13, 2021

It's been 2 years (wow) but there have been some changes:

In my model, local is the same as accepted. So it only really makes sense to diff against upstream.

This is no longer true. there are now 3 states local accepted and upstream. accepted is marked by a git ref called AUR_SEEN.
This ref is also supported by aurutils, pacaur and paru (as a user of aur-fetch).

This is nice because you can merge the changes, hand of the merged packagebuild to the user and if rua exists it's still not marked as seen. Also the grouping of packages in cache (which you mentioned rua does) allows for a unified cache and diffs. paru, pacaur, aurutils and maybe other's I've forgotten use AUR_DEST as an environment to control the aur cache.

If you want to work on merging this again I'd be happy to work on it. If not, it would be nice for rua to implement AUR_SEEN and AURDEST.

@Morganamilo
Copy link
Collaborator Author

I'm not sure if rua does nuke the cache after building still though. There should be no need for it though as you can use BUILDIR and PKGDEST and such to control output. at a stretch you could copy the pkgbuild to a dir as they are tiny.

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.

2 participants