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

CI: Use PR fork of libmypaint #784

Open
odysseywestra opened this issue Feb 12, 2017 · 9 comments
Open

CI: Use PR fork of libmypaint #784

odysseywestra opened this issue Feb 12, 2017 · 9 comments
Assignees
Labels
cat.Packaging Issue relates to building/packaging type.Enhancement Issue requests feature.
Milestone

Comments

@odysseywestra
Copy link
Member

odysseywestra commented Feb 12, 2017

Description of the problem

I'm noticing that some of the builds are failing due to them not using the Contributor's Fork of libmypaint. Travis-CI and related Continuous Intergration(CI) tools should use the fork first. If the fork of libmypaint is not available, the CI tools should default to upstream.

This can also benefit the use case of Pull Requests for libmypaint as well. Since commit 2612c4a, MyPaint itself has had the ability to generate Appimages on Travis-CI and upload them to transfer.sh. This allows Testers to download the Appimage and review the Pull Request without having to build from source. This functionality could expand to test builds for windows in the future well.

So in this use case, contributors who make a pull request for libmypaint, could make a [WIP] Pull Request on MyPaint itself. This would allow CI tools would build with the Contributor's fork of libmypaint, run tests and transfer builds to transfer.sh so user can download and test the builds.

Affected Pull Requests

@briend - #731 with mypaint/libmypaint#70
@ShadowKyogre - #720 with mypaint/libmypaint#57

Steps to reproduce

  1. Create pull request on mypaint/libmypaint with changes to libmypaint
  2. Create second pull request on mypaint/mypaint with changes required by libmypaint
  3. CI Tools will fail to build on mypaint/mypaint.

Backtraces or error messages

See CI logs from #731

@achadwick
Copy link
Member

This used to be solvable with submodules and relative references, amusingly. Although doing a squash-merge would still make a mockery of the contributor's refs, meaning a manual fixup process.

There are several practical challenges to doing this:

  • We cannot predict what the contributor's branches are going to be called
    • Does their mypaint branch "something1" require their libmypaint branch "something2"?
    • Or just their libmypaint master?
  • We cannot guarantee that their libmypaint master branch will work with their mypaint working branch.
  • I prefer just saying "and mypaint will build your libmypaint fork's master if it exists" though.
    • Reason: it's simple to explain. If they get the arrangement of their branches wrong, it's a manual process for redoing the checking builds.
    • Just like now, in fact...
  • How to convert the checked-out repo's absolute remote URI for the contributor's mypaint fork into an absolute ref for any libmypaint fork they might happen to have
    • I suppose you start by looking at the output of git remote
    • There is no way of predicting what the CI box's remote is called. Is it always origin? Doesn't have to be.

Not insurmountable, though!

@odysseywestra
Copy link
Member Author

odysseywestra commented Feb 15, 2017

@odysseywestra
Copy link
Member Author

odysseywestra commented Feb 15, 2017

Nvm, that really won't help.. in our use case subtrees will just create a bigger mess than we need.

Sadly after all the research I did on this, submodules are technically the only way to solve this. Unless I'm missing something.

Something we don't want to use.

It's either that or just have the user manually specify which repository to pull from in the .yaml in a single commit and then revert that commit when it comes to merging in the pull request.

@odysseywestra
Copy link
Member Author

odysseywestra commented Feb 16, 2017

I have an idea.

Instead of the user pull requesting into libmypaint master, what about having them pull request and merge into a testing or unstable branch for example? That way we will know what branch it is in and can point the CI to the correct branch. It may add an extra step in the process, but at least we can have more developers working on a testing branch if they are part of the developers team.

Basically we would be changing our work flow from fork and pull request to branching and pull request.

Okay enough of hair brain ideas from me. Off to bed.

@odysseywestra
Copy link
Member Author

Sorry pressed wrong button.

@odysseywestra
Copy link
Member Author

odysseywestra commented Apr 6, 2017

I guess a good way to combat this would be to adopt git flow style approach. That way we know what the branches names are and keep them the same in both repositories.

The reason I say this is because of this: https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions

@odysseywestra odysseywestra self-assigned this Dec 27, 2019
@AesaraB AesaraB added type.Enhancement Issue requests feature. cat.Packaging Issue relates to building/packaging info.Triage Need to triage this labels Jan 17, 2024
@AesaraB AesaraB added this to the v2.02 milestone Jan 17, 2024
@AesaraB AesaraB removed the info.Triage Need to triage this label Jan 17, 2024
@AesaraB AesaraB changed the title Continous Intergration Tools should use Contributors Fork of libmypaint First. Building: Make CI tools use PR fork of libmypaint Jan 18, 2024
@AesaraB AesaraB assigned AesaraB and unassigned odysseywestra Jan 18, 2024
@AesaraB AesaraB removed their assignment Feb 2, 2024
@jtojnar
Copy link
Contributor

jtojnar commented Feb 6, 2024

To sum up the proposed options (adding unmentioned monorepo for completeness):

  • add libmypaint as a submodule/subtree to mypaint, and temporarily retarget it to a fork in PR branch
    • annoying: submodule needs to be updated everytime libmypaint is updated
  • temporarily copy libmypaint to mypaint repo
    • even more annoying: same as above and we cannot even use git to handle the update
  • require merging changes into testing branch in the upstream libmypaint repo
    • slow iteration: requires someone from the project merging
    • multiple people can step on each other toes when developing simultaneously
  • merge mypaint, libmypaint and ¿mypaint-brushes? into a single big monorepo
    • best developer experience, easy to run integration CI pipeline in both directions
    • requires users to clone larger git repos (or use subtree clones which is not well known, or use tarballs)
    • since git tags are repo-wide, independent evolution is fuzzy
  • test against libmypaint from matching branch in same user’s fork
    • this could work but we would need to check if the branch exists
    • requires discipline since GitHub does not allow renaming branches for open PRs
  • require temporarily changing the libmypaint repo URL in a config file
    • I like this the most, it is explicit and understandable

If we switch MyPaint to Meson, I think the winning combination might be using a Meson subproject for the dependencies. Specifically a wrap file like this one.

Subprojects allow using dependencies from the system manager while transparently falling back to cloning a git repo when not present. We can specify a branch in the wrap file and if a developer needs a change from their beanch, they could temporarily override it. Delegating it to Meson would allow a single place for the change both in CI and locally for users/developers.

Downside, like with the explicit clone-ing we do now, is that CI runs would not be reproducible. (When we restart the action, someone might have pushed to the libmypaint repo in the meanwhile.) But that is not solvable without pinning like submodules do (which is annoying to manage).

@odysseywestra
Copy link
Member Author

odysseywestra commented Feb 6, 2024

Yeah, the third option would be what I was looking for. That was my original intention of merging initial PRs into Feature Branches. The feature branch can be created after the PR is submitted and the PR can be edited to point to the correct branch after it's created. That will give those in the Core team to prioritize what parts are fully merged into the main branch if they want to chime in. So basically my job todo atm.

When MyPaint was built with SCONS, submodules were used which caused a lot of confusion among developers and thus breaking the build chain. So when we moved to distutils and setup.py, we made it a goal to do away with submodules. Also, we can't merge all three repos into a single repo since other projects use libmypaint(gimp from what I recall) and specifically asked for libmypaint and the brushes to be separated from MyPaint itself.

Meson subproject seems like a good option. Like I said in #1229, as long as we keep it mostly the same process as we have it now with the current build system, that will help foster consistency while also adopting new standards. Yes, we would need to make sure the CI tools use the same git branch in both projects. That will probably be a challenge but should be easier to do since we would be mainly keeping everything within the repos and not using the CI tools except for Hound to deal with the initial PRs. I don't think we need to worry about the brushes since they are mainly config files and modular in a sense.

Hopefully what I'm saying makes sense. I'm mainly just taking what I've observed over time with MyPaint and making sure we don't fall back to methods that didn't work for it when I got involved.

Edit: meant to say third option...

@odysseywestra
Copy link
Member Author

I am working on a new build script that should address this. It works with forks and branches, but I haven't tested it with pull requests yet.

https://github.com/odysseywestra/mypaint/tree/github-actions-script-build

Still needs a lot of work, but I'm making progress.

@AesaraB AesaraB changed the title Building: Make CI tools use PR fork of libmypaint CI: Use PR fork of libmypaint Feb 24, 2024
@AesaraB AesaraB modified the milestones: v3.0.0, v2.1.0 Feb 24, 2024
@AesaraB AesaraB self-assigned this Feb 24, 2024
@AesaraB AesaraB assigned odysseywestra and unassigned AesaraB Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat.Packaging Issue relates to building/packaging type.Enhancement Issue requests feature.
Development

No branches or pull requests

4 participants