Skip to content

Merge new upstream stubs in master #42

@stlehmann

Description

@stlehmann
Collaborator

Thanks to @BryceBeagle we now have much more stubs to offer. I didn't have a closer look at them. But I guess they also need to be altered to be usable with mypy.

Activity

stlehmann

stlehmann commented on May 1, 2020

@stlehmann
CollaboratorAuthor

It might be a very pragmatic approach but for the new stubs I would maybe just take them and put them in the master branch, release them and fix errors on-the-go. Otherwise the work might be a bit overwhelming. @BryceBeagle, @The-Compiler what do you think?

BryceBeagle

BryceBeagle commented on May 2, 2020

@BryceBeagle
Collaborator

I think the best long term solution is to have one branch. I've been thinking about a potential workflow for a while and would like to hear your opinions.

  • The upstream stubs live on the master branch
  • The changes we make don't actually get applied to the upstream stubs, but instead reside in a set of patch files
  • The patch files could correspond to (and better document) the issues that currently exist in the issues.md
  • When a new version of PyQt is released, just make sure the patch files still properly apply by fixing the (presumably) minor changes that occurred
  • When we want to release a new version of the patched stubs, we can just apply the patches locally and then build/deploy a wheel from those files
  • Tests could be written such that we test to make sure a mypy error occurs without the patches, and then does not occur after a patch has been applied

* We might even not want to store the upstream stubs, but only store the patches. A repo initialization script could just build the stubs with the Dockerfile (or pull from the PyPI wheels when they contain the stubs) for development purposes.

The-Compiler

The-Compiler commented on May 2, 2020

@The-Compiler
Collaborator

@stlehmann Fixing errors as they come up sounds okay - there are probably a lot of unfixed issues (like more signals) in the existing stubs anyways.

@BryceBeagle As long as we hand-edit the stubs (rather than automating the edits in some way), I'd rather keep the current workflow. Mixing patch-files and git just seems to complicate the workflow for everyone - it's much harder for contributors to edit, it's harder to review, and it's much harder to get the end result. I'd rather use git's functionality to get me diffs in whatever way seems to help, rather than storing diffs in a git repo.

stlehmann

stlehmann commented on May 2, 2020

@stlehmann
CollaboratorAuthor

I actually like the idea of @BryceBeagle to use patch files. At the moment I find it VERY cumbersome to go through all the code and see what has changed after each release. We actually have no overview which changes have been applied. But I agree with @The-Compiler that using git patch files would make things even more complicated because they are hard to maintain.
I would prefer a much clearer format that allows easy editing and provides a clear view what changes have been appied and where.

How about a JSON-file with the following format:

{
    "file": "QtWidgets.pyi",
    "patches": [
        {
            "operation": "edit"  # can be edit or insert
            "line": 51,
            "org": "def windowIconTextChanged(self, iconText: str) -> None: ..."
            "new": "windowIconChanged: QtCore.pyqtSignal"
        },
    ]
}

We always check if the given line contains the original content. This way we will recognize if content has been moved in the original stubs, e.g. if a new line has been inserted.

The-Compiler

The-Compiler commented on May 2, 2020

@The-Compiler
Collaborator

At the moment I find it VERY cumbersome to go through all the code and see what has changed after each release. We actually have no overview which changes have been applied.

Doesn't commands like git diff upstream..master or git show upstream etc. give you exactly that?

I don't see the benefits to write a custom patch format if it just replaces lines. If we're going to write a custom patcher, IMHO it should be more "semantic", i.e. only take a list of all signals as input and then edit the stubs on its own based on that (essentially option 2 in #6 (comment))

stlehmann

stlehmann commented on May 2, 2020

@stlehmann
CollaboratorAuthor

I don't see the benefits to write a custom patch format if it just replaces lines.

True, if you phrase it like this it might not be worth the effort. Though I'm still not satisfied with the current workflow.

take a list of all signals as input and then edit the stubs on its own based on that (essentially option 2 in #6 (comment))

I like the idea of that. That would take a huge effort away from us.

stlehmann

stlehmann commented on May 4, 2020

@stlehmann
CollaboratorAuthor

#44 merged the new stubs files including QtWebkit and Qt3D. Now our CI will of course throw endless errors as the new stubs are not edited, yet.

The-Compiler

The-Compiler commented on May 4, 2020

@The-Compiler
Collaborator

I'll continue the automation discussion in #6.

altendky

altendky commented on Sep 13, 2020

@altendky
Collaborator

What is the current workflow? One branch for upstream and one branch for our changes? When upstream updates, commit to the upstream branch then merge into our branch? or...

altendky

altendky commented on Sep 14, 2020

@altendky
Collaborator

I'm going through the merge of existing upstream to master and it's a mess. The merge base has a bunch of the customizations in it. I haven't dug into the history but I think we need to make sure this doesn't happen in the future. Git needs to be able to see a clean diff between the merge base and upstream that contains only actual upstream changes. Presently it sees that upstream changed our customizations and so it thinks upstream needs to bring a change into the merge which makes for piles of conflicts. I believe the rule is 'never merge non-upstream into upstream'. Upstream needs to stay pure.

(or maybe I still haven't figured out the workflow here)

altendky

altendky commented on Sep 14, 2020

@altendky
Collaborator

So I think the auto-merge probably threw away any of our customizations that we hadn't changed 'recently' due to only upstream having 'changed' them. It looks like maybe just 2ce7aa9 is the problem so I'll try merging from right before that instead and see what happens.

stlehmann

stlehmann commented on Sep 14, 2020

@stlehmann
CollaboratorAuthor

What is the current workflow? One branch for upstream and one branch for our changes? When upstream updates, commit to the upstream branch then merge into our branch? or...

@altendky Yes, basically the Upstream branch contains the built and unchanged stubs from the PyQt5 repository. If there are changes on the Upstream branch they need to be merged in the master branch.

Upstream needs to stay pure.

Yes, that is basically the idea.

stlehmann

stlehmann commented on Sep 14, 2020

@stlehmann
CollaboratorAuthor

Are you refering to PR #64? It looks good for me on first glance. Where are you having trouble specifically?

altendky

altendky commented on Sep 14, 2020

@altendky
Collaborator

No, those were straightforward. I haven't pushed anything related to the troublesome merge. Merging upstream into master (even without either 5.15) is a mess because this is actually the first time that has happened. It's looking a lot cleaner now that I'm excluding the dummy merge of master into upstream, but still not great. I think that actually the very first upstream commit should have been dummy merged back into master immediately so that git would realize it is included in master. Then any future merge of upstream into master would exclusively merge actual upstream changes since the merge base would be on upstream, not master. Trying that now...

altendky

altendky commented on Sep 14, 2020

@altendky
Collaborator

Yeah, that was a way smoother merge, even though it was 'three'. See #63

  1. Ignore merge original upstream to master
  2. Real merge almost latest upstream to master
  3. Ignore merge latest upstream to master (because it was just an ignore merge itself)

I'll go back and try to figure out the SIP stub build, then update the 5.15.0 and 5.15.1 branches accordingly.

altendky

altendky commented on Sep 14, 2020

@altendky
Collaborator

Ok... So without presuming I've done everything right, here's what I see as the groupings to go through my PRs. Each group should be completed before working on the next. Hopefully I'm going down the expected and desired path with this work.

  1. Tool enhancements
  2. Update upstream
  3. Update master
    • Merge upstream 5.15.0 #70 Merge 5.15.0 to master
      Merge upstream 5.15.1 #71 Merge 5.15.1 to master
  4. The other fixups
stlehmann

stlehmann commented on Sep 18, 2020

@stlehmann
CollaboratorAuthor

@altendky, @BryceBeagle As you are both highly engaged I invited both of you as collaborators of this project. I hope this is OK for you and adds to the progress of this project :)

altendky

altendky commented on Sep 22, 2020

@altendky
Collaborator

@BryceBeagle, thanks for all the reviews. I definitely owe you... something. Let me know if there's something else I can contribute reviews or code or whatever to. (or pieces you would like to fix here and have me review...)

BryceBeagle

BryceBeagle commented on Sep 22, 2020

@BryceBeagle
Collaborator

Reviewing is the easy part 👍

altendky

altendky commented on Sep 22, 2020

@altendky
Collaborator

With #71 merged to master this should be done. I don't know how releases have been managed but I expect to keep making progress on the other bits in the near future if that matters.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    help wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @altendky@The-Compiler@stlehmann@BryceBeagle

        Issue actions

          Merge new upstream stubs in master · Issue #42 · python-qt-tools/PyQt5-stubs