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

Drop old common data #1676

Closed
wants to merge 1 commit into from
Closed

Drop old common data #1676

wants to merge 1 commit into from

Conversation

alecandido
Copy link
Member

No description provided.

alecandido added a commit that referenced this pull request Feb 22, 2023
@alecandido alecandido changed the base branch from master to NEW_COMMONDATA February 22, 2023 10:31
@Zaharid Zaharid marked this pull request as draft February 22, 2023 17:53
@alecandido
Copy link
Member Author

@Zaharid this PR will not receive any new commit, it is ready as it is (split from #1500 to keep this big change separate).

Why did you mark this as draft?

@Zaharid
Copy link
Contributor

Zaharid commented Feb 23, 2023

I think we need to discuss how and when the old commondata is to be removed. I don't believe #1500 was ever intended to be merged as much for playing around with the new format.

I think ideally we'd want to see plots of old vs new produced with one given version of the code that contains all.
cc @enocera.

@alecandido
Copy link
Member Author

I think we need to discuss how and when the old commondata is to be removed. I don't believe #1500 was ever intended to be merged as much for playing around with the new format.

Fine, but it was not intended to be merged even if it was not draft.

For me, the meaning of draft is "work in progress, not ready yet". This is ready as it is, and if the main point is "do not merge right now", it is sufficient not to do it, for as long as we do not agree it is time to merge.

I perfectly agree on the "not right now", I do not want to see this in master for as long as the transition is not complete, that's why I split in the first place. It's just that I believe the draft mark to be misused here, and inconsistently with other PRs.

@Zaharid
Copy link
Contributor

Zaharid commented Feb 23, 2023

I perfectly agree on the "not right now", I do not want to see this in master for as long as the transition is not complete, that's why I split in the first place. It's just that I believe the draft mark to be misused here, and inconsistently with other PRs.

I agree. We don't have the new files yet, and have had little to no discussion as to what to do with the old ones. Hence, I think this is premature and having this around, in draft form or otherwise, might be a way to drag code meetings. It can be revisited.

Also, eventually, removing the commondata should be re-done against master rather than the branch in #1500. At the moment the Files Changed tab in this PR looks very confusing (it is only removing the new commondatata functionality and seemingly not touching the old), and upon merging to master, it results in a funny history with new commondata functionality added and then deleted.

@Zaharid Zaharid closed this Feb 23, 2023
@alecandido alecandido reopened this Feb 23, 2023
@alecandido alecandido marked this pull request as ready for review February 23, 2023 16:13
@scarlehoff scarlehoff marked this pull request as draft February 23, 2023 16:15
@alecandido
Copy link
Member Author

alecandido commented Feb 23, 2023

@Zaharid yesterday an assembly of people at which, you decided not to attend, chose altogether to have this PR (someone agreed, and no one was against), and to keep here that "confusing Files Changed tab" in order not to confuse any other PR, since 2k files changed for sure would hide any useful change.

This is the purpose of this PR, and it is a clear and simple purpose.

Please, do not feel free to close PRs about which there is a shared agreement but you. And please, do not feel free to close any other issue just because it is not matching your personal taste and opinion.

@alecandido alecandido marked this pull request as ready for review February 23, 2023 16:17
@scarlehoff
Copy link
Member

I prefer the draft just to make sure I don't press the "Merge pull request" by mistake. It wouldn't be the first time, better not to risk it.
Let's leave this PR as a draft so that we see the "Drop old common data" at every Phone Call so that we are constantly reminded.

@scarlehoff scarlehoff marked this pull request as draft February 23, 2023 16:17
@alecandido
Copy link
Member Author

At the moment the Files Changed tab in this PR looks very confusing (it is only removing the new commondatata functionality and seemingly not touching the old), and upon merging to master, it results in a funny history with new commondata functionality added and then deleted.

I accept useful observations: I can simply rebase on master and change the history to have a useful one.
However, since this is not close to be merged, it is definitely not an urgent operation.

I prefer the draft just to make sure I don't press the "Merge pull request" by mistake. It wouldn't be the first time, better not to risk it.
Let's leave this PR as a draft so that we see the "Drop old common data" at every Phone Call so that we are constantly reminded.

Objection accepted

@alecandido alecandido changed the base branch from NEW_COMMONDATA to master February 23, 2023 16:32
@Zaharid
Copy link
Contributor

Zaharid commented Feb 23, 2023

I am sorry but not. If you want to have a remainder to delete the old commondata, an issue is appropriate. It would a very useful to discuss what to do when, but rather far away from any concrete pr.

As it is, this should not be merged against master for the reason I gave, and should not be merged against #1500 because it reverts all relevant additions, so it needs to be redone regardless of everything else. I feel I spent a rather long time trying to understand what this does, how it merges against master, and giving a reasoned reply as to what to do. I feel that tagging zillions of people and reopening unrelated issues is a very unhelpful way to proceed and certainly a terrible way to change my mind.

@alecandido
Copy link
Member Author

I accept useful observations: I can simply rebase on master and change the history to have a useful one.
However, since this is not close to be merged, it is definitely not an urgent operation.

Actually, it was so simple that I've done immediately :)
Thanks git rebase!

@alecandido
Copy link
Member Author

tagging zillions of people

Not done at random: they are the people that discussed this yesterday. I've created this PR during the meeting itself, just following the agreement we had.
It was not a personal decision, so I don't see why I should keep discussing in private.

@scarlehoff
Copy link
Member

I'm going to close this for the time being because sadly there'd been additions to the commondata (which results in conflicts).
The idea right now is to only drop the old buildmaster once (if?) #1709 is completed.

@scarlehoff scarlehoff closed this Nov 13, 2023
@scarlehoff scarlehoff deleted the drop-old-buildmaster branch November 13, 2023 09:05
@alecandido
Copy link
Member Author

This PR was just taking away rm -rf buildmaster from #1500, it will take nothing to redo once we're ready (the goal was just not to have it there)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants