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

Branching strategy #31

Open
Rot127 opened this issue Sep 4, 2023 · 12 comments
Open

Branching strategy #31

Rot127 opened this issue Sep 4, 2023 · 12 comments

Comments

@Rot127
Copy link

Rot127 commented Sep 4, 2023

I (tried) to rebase the tracewrap-6.20 branch onto the newest stable QEMU and it is not fun.
There are a bunch of conflicts.

I suggest we agree on a branch strategy. Because if one person does the rebase and has to fix 2+ architectures every time, we definitely end up with false tracing (because we have no proper tests).

How about we have a main branch which only implements the core of tracewrap.

We can call it tracewrap-X.Y, which is the current newest stable branch of QEMU + one or few more commits with the core tracewrap code.
This one is easily updated if QEMU has a new release. It could (should?) be even automated with the CI.

From this every arch branches. If some needs to trace a specific arch they select the archs branch.
If it needs a rebase onto the newest tracewrap-X.Y, fine. But since the user will use it anyway, it gets directly tested.

cc @thestr4ng3r @DMaroo Since ARM and x86/i386 had many many conflicts.

@Rot127
Copy link
Author

Rot127 commented Sep 4, 2023

@XVilka @ivg Would you mind pushing QEMUs latest stable here? I'd like to open a PR onto it with tracewrap.

@DMaroo
Copy link
Member

DMaroo commented Sep 5, 2023

That's a good idea, makes sense to me.

@XVilka
Copy link

XVilka commented Sep 5, 2023

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

@XVilka
Copy link

XVilka commented Sep 5, 2023

In the future, as TCG plugins API almost passed the review, I hope it would alleviate the pain by just using the API instead: #24

@DMaroo
Copy link
Member

DMaroo commented Sep 5, 2023

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

This would be relatively easy since all the branches for individual archs would (likely) not share any files/code. Hence, no merge conflicts in merging them all to have a combined branch.

@Rot127
Copy link
Author

Rot127 commented Sep 5, 2023

Hence, no merge conflicts in merging them all to have a combined branch.

The merge conflicts come from our tracewrap additions in the translate.c files. For very active architectures, there are a lot of changes in between releases and then we get again many merge conflicts.

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

Why is this necessary? Some architectures have no additions for several releases. For them it is not worth the effort to rebase them onto the newest one IMHO.
So where would be the advantage of having a single all containing branch?

@XVilka
Copy link

XVilka commented Sep 6, 2023

Why is this necessary? Some architectures have no additions for several releases. For them it is not worth the effort to rebase them onto the newest one IMHO. So where would be the advantage of having a single all containing branch?

It's not uncommon for QEMU to perform tree-wide changes to the TCG uplifters. Recent examples are atomicity changes,
new negsetcond opcode:

@thestr4ng3r
Copy link

We can call it tracewrap-X.Y, which is the current newest stable branch of QEMU + one or few more commits with the core tracewrap code.

I propose tracewrap-X.Y-base to clearly indicate this is only the base with no added usable tracing by itself.

Idea is interesting but we need also a branch that is sum of all these architecture branches - something like tracewrap-8.2-all

Wouldn't this be essentially the same as we have now, i.e. one branch with everything in it?

I am not yet fully convinced that the proposed approach will have more benefits (independent and thus easier rebasing per architecture) than downsides (more branches to handle, having to merge back core changes from architecture branches if there are any), but it could be a worthwhile experiment to try this approach for a new rebase to see how well it works in practice.

@DMaroo
Copy link
Member

DMaroo commented Sep 7, 2023

For very active architectures, there are a lot of changes in between releases and then we get again many merge conflicts.

Agreed, but there won't be any inter-arch merge conflicts (since each of those translate.c files is under its own arch directory), which is what I meant in that comment. So we could have a combined branch by merging all the separate arch branches without any conflicts.

We would just need to make sure that all the individual branches are conflict free and up to date, then the combined branch would also be conflict free and up to date.

@Rot127
Copy link
Author

Rot127 commented Sep 7, 2023

We would just need to make sure that all the individual branches are conflict free and up to date,

Yes. This is the work intensive part. If we can agree that everyone rebases their branch on top on the newest QEMU release, we are good.

@XVilka
Copy link

XVilka commented Sep 11, 2023

@Rot127 I pushed the updated master and stable-8.1 from the latest QEMU

@Rot127 Rot127 mentioned this issue Oct 26, 2023
@ivg
Copy link
Member

ivg commented Oct 26, 2023

I have no objections to the proposed branching strategy. However, once we finalize it, let's write it in some document, either a wiki page or in a CONTRIBUTING.md file, your choice.

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

No branches or pull requests

5 participants