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

djview4: drop GCC dependency #110664

Closed
wants to merge 1 commit into from
Closed

Conversation

danielnachun
Copy link
Member

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@iMichka iMichka added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Sep 14, 2022
@danielnachun
Copy link
Member Author

Why is this failing on 11-arm64 but working on 11 and 12-arm64? That seems to imply the failure is not because of Big Sur or building on ARM, which doesn't really make sense.

@danielnachun danielnachun added the CI-no-bottles Merge without publishing bottles label Oct 5, 2022
@danielnachun
Copy link
Member Author

@Homebrew/core, since this is only failing on macOS, I am thinking maybe we should delete the Linux bottle, merge this as syntax-only, and then rebottle on Linux. It's a very quick formula to rebuild and would get rid of the GCC 11 references for good.

@cho-m
Copy link
Member

cho-m commented Oct 5, 2022

@Homebrew/core, since this is only failing on macOS, I am thinking maybe we should delete the Linux bottle, merge this as syntax-only, and then rebottle on Linux. It's a very quick formula to rebuild and would get rid of the GCC 11 references for good.

Is it possible to drop a single bottle from existing version in GitHub package?

I know we can delete the entire version. I think we tried to delete a single uploaded sha256 before but it didn't work.

EDIT: Though not sure how new Ubuntu impacts this as existing Linux bottle is currently linux/ubuntu 16.04.7/amd64 so may be a separate upload would work?

@Bo98
Copy link
Member

Bo98 commented Oct 5, 2022

Removing the 2>&1 from $QMAKE > conftest.out 2>&1 in the configure script probably fixes the issue.

Is it possible to drop a single bottle from existing version in GitHub package?

No, we do not support doing this.

@danielnachun
Copy link
Member Author

I should have said bottle line for Linux, not the actual bottle. But I will try the fix suggested by @Bo98 first.

@Bo98
Copy link
Member

Bo98 commented Oct 5, 2022

I should have said bottle line for Linux, not the actual bottle.

When building again, you would be producing a bottle with an unchanged rebuild number, which will clash on upload (probably).

@danielnachun
Copy link
Member Author

When building again, you would be producing a bottle with an unchanged rebuild number, which will clash on upload (probably).

Ah that's good to know why that doesn't work. Maybe we can just merge this CI-no-bottles? I'm not sure rebuilding on Linux is necessary for this.

@danielnachun
Copy link
Member Author

Removing the 2>&1 from $QMAKE > conftest.out 2>&1 in the configure script probably fixes the issue.

That seems to break the build unfortunately. I'm still puzzled as to why this only happens on a single platform.

@cho-m
Copy link
Member

cho-m commented Oct 5, 2022

The conflict warning (perhaps from more than one of XCode/CLT/etc. installed) has only been consistently seen on Big Sur ARM runner. It had broken some formulae that run scripts that don't know how to handle these warnings, for example:

  • chapel - it has a test script that expects no stdout/stderr
  • mpv - macOS SDK version detection parses first line of output or something like that.

Looks like qmake outputs to stderr which is why it broke. The QMake check is quite flaky. May need to filter out some warning lines, or patch out check, or upstream a more stable check:

  if ( cd conftest.d && $QMAKE > conftest.out 2>&1 ) ; then
    sed -e 's/^.*: *//' < conftest.d/conftest.out > conftest.d/conftest.sh
    . conftest.d/conftest.sh
    rm -rf conftest.d

@Bo98
Copy link
Member

Bo98 commented Oct 5, 2022

Yeah looks like anything that's not starting with Project MESSAGE: should be stripped from stderr.

@danielnachun
Copy link
Member Author

The conflict warning (perhaps from more than one of XCode/CLT/etc. installed) has only been consistently seen on Big Sur ARM runner. It had broken some formulae that run scripts that don't know how to handle these warnings, for example:

  • chapel - it has a test script that expects no stdout/stderr
  • mpv - macOS SDK version detection parses first line of output or something like that.

Looks like qmake outputs to stderr which is why it broke. The QMake check is quite flaky. May need to filter out some warning lines, or patch out check, or upstream a more stable check:

  if ( cd conftest.d && $QMAKE > conftest.out 2>&1 ) ; then
    sed -e 's/^.*: *//' < conftest.d/conftest.out > conftest.d/conftest.sh
    . conftest.d/conftest.sh
    rm -rf conftest.d

I'll test that possible replacement locally and upstream it if it works.

@danielnachun
Copy link
Member Author

Thanks @cho-m for pointing me to that code block and and @Bo98 for for the tip about filtering out any lines not containing Project MESSAGE: - a simple grep was able to fix this! I will upstream this as a patch and add the link and this should finally be ready to go.

@danielnachun
Copy link
Member Author

I upstreamed the fix here and added a link in a comment. This should be ready to merge.

@danielnachun danielnachun added the ready to merge PR can be merged once CI is green label Oct 6, 2022
@carlocab
Copy link
Member

carlocab commented Oct 6, 2022

I should have said bottle line for Linux, not the actual bottle.

When building again, you would be producing a bottle with an unchanged rebuild number, which will clash on upload (probably).

Actually, brew/skopeo will quietly replace the existing bottle.

This is why we have issues with duplicate PRs that are merged simultaneously -- the bottle commit from the first is pushed, but the second replaces the uploaded bottles.

@cho-m
Copy link
Member

cho-m commented Oct 6, 2022

I should have said bottle line for Linux, not the actual bottle.

When building again, you would be producing a bottle with an unchanged rebuild number, which will clash on upload (probably).

Actually, brew/skopeo will quietly replace the existing bottle.

This is why we have issues with duplicate PRs that are merged simultaneously -- the bottle commit from the first is pushed, but the second replaces the uploaded bottles.

In past I don't remember this happening. During initial Linux bottling for homebrew/core, I needed to rebottle only Linux and recall trying to removing bottle line from a formula, deleting SHA from Github packages, and all sorts of other work arounds. Test-bot and other dev upload commands ended up rejecting the new upload.

Not sure if this has changed or if this happens in race condition with simultaneous upload to GitHub packages.

The only time I could get GitHub packages to accept a new upload was when the previously uploaded version was deleted. This happened a few times when test-bot flaked and failed partway through upload and left behind incomplete uploads. Uploads were rejected until the previous ones were deleted.

@danielnachun
Copy link
Member Author

Could that be because the manifest needs to be updated? But I seem to recall as well that if you just delete the bottle line, you can then dispatch a new bottle even if it's for the same version. If that's not true we should fix that as it should not be so difficult to just rebottle something on only one or a subset of the tags.

@Bo98
Copy link
Member

Bo98 commented Oct 6, 2022

Could that be because the manifest needs to be updated? But I seem to recall as well that if you just delete the bottle line, you can then dispatch a new bottle even if it's for the same version. If that's not true we should fix that as it should not be so difficult to just rebottle something on only one or a subset of the tags.

This defeats the whole point of the rebuild. If we want to delete old bottles then what's the point of rebuild?

@danielnachun danielnachun requested a review from a team October 6, 2022 16:57
@MikeMcQuaid
Copy link
Member

@Bo98 if you changed the SHA of a bottle without a rebuild previously everyone with the existing bottle would get a local checksum failure. If that's no longer the case under GitHub Packages: perhaps we can adapt our flow and avoid rebuild or revision as often.

If that's still the case: it's a really bad idea to ever remove bottles and force these checksum mismatches on users.

@danielnachun
Copy link
Member Author

@Bo98 if you changed the SHA of a bottle without a rebuild previously everyone with the existing bottle would get a local checksum failure. If that's no longer the case under GitHub Packages: perhaps we can adapt our flow and avoid rebuild or revision as often.

If that's still the case: it's a really bad idea to ever remove bottles and force these checksum mismatches on users.

This is what happens if you remove a bottle line and try to rebuild it with dispatch bottle: https://github.com/Homebrew/homebrew-core/actions/runs/3199468970. It still has the old checksum and doesn't allow it to be overwritten, presumably to prevent the checksum mismatch for existing installations. Maybe what we need is some sort of a DSL that says "if you already have this bottle installed and the checksum mismatches, automatically reinstall it". That could allow us to update individual platforms without forcing an update on unaffected platforms.

Right now we only do rebuilds for all platforms, which probably mades sense when we only had macOS in homebrew/core, where you would rarely if ever need to only rebuild a bottle for 1 tag. But now that we also have Linux, we will have many cases where a bottle only needs to be rebuilt for one OS. We could have OS-specific revisions but there were previously objections to doing that.

@MikeMcQuaid
Copy link
Member

It still has the old checksum and doesn't allow it to be overwritten, presumably to prevent the checksum mismatch for existing installations

Yes.

Maybe what we need is some sort of a DSL that says "if you already have this bottle installed and the checksum mismatches, automatically reinstall it".

Definitely not. This would be a terrible idea from a security perspective, sorry.

That could allow us to update individual platforms without forcing an update on unaffected platforms.

I opened Homebrew/brew#13877 for this. I'd suggest moving forward with that approach if we want to do this.

Right now we only do rebuilds for all platforms, which probably mades sense when we only had macOS in homebrew/core, where you would rarely if ever need to only rebuild a bottle for 1 tag.

We didn't do that for macOS either, we always built a new bottle just for the affected OS. The issue is that on Linux we didn't add a new bottle definition for this. Again: the PR above could be resurrected to follow this approach and trivially fall back to the existing bottles.

We could have OS-specific revisions but there were previously objections to doing that.

We shouldn't do this either. We don't want revisions just for bottle rebuilds. We don't want to force a reinstall on people building from source just because we've built a new bottle.

@danielnachun
Copy link
Member Author

It still has the old checksum and doesn't allow it to be overwritten, presumably to prevent the checksum mismatch for existing installations

Yes.

Maybe what we need is some sort of a DSL that says "if you already have this bottle installed and the checksum mismatches, automatically reinstall it".

Definitely not. This would be a terrible idea from a security perspective, sorry.

That could allow us to update individual platforms without forcing an update on unaffected platforms.

I opened Homebrew/brew#13877 for this. I'd suggest moving forward with that approach if we want to do this.

Right now we only do rebuilds for all platforms, which probably mades sense when we only had macOS in homebrew/core, where you would rarely if ever need to only rebuild a bottle for 1 tag.

We didn't do that for macOS either, we always built a new bottle just for the affected OS. The issue is that on Linux we didn't add a new bottle definition for this. Again: the PR above could be resurrected to follow this approach and trivially fall back to the existing bottles.

We could have OS-specific revisions but there were previously objections to doing that.

We shouldn't do this either. We don't want revisions just for bottle rebuilds. We don't want to force a reinstall on people building from source just because we've built a new bottle.

The new tag only helps if the bottle needs a one off rebuild for the migration, but I was thinking more about ways to selectively rebuild bottles in the longer term.

Most of the time it seems simply rebuilding the bottle without a revision bump is all that we really need to do. That doesn't force anybody to reinstall or update. The main question that seems somewhat unresolved is how aggressively we need to do dependent testing if the only thing we are doing is rebuilding the bottle without changing anything in the formula.

The simple approach that I think would work for this is just making a "dummy PR" that only adds a comment, let it run dependent testing, and if nothing breaks then close the PR and just use the dispatch rebottle workflow we already have to rebuild without revision bumping.

@MikeMcQuaid
Copy link
Member

Most of the time it seems simply rebuilding the bottle without a revision bump is all that we really need to do.

Why/when do we need to rebuild the bottle in these cases rather than just waiting for the next revision/version to do so?

The simple approach that I think would work for this is just making a "dummy PR" that only adds a comment, let it run dependent testing, and if nothing breaks then close the PR and just use the dispatch rebottle workflow we already have to rebuild without revision bumping.

I thought we've agreed that we can selectively rebuild bottles like this?

@danielnachun
Copy link
Member Author

Most of the time it seems simply rebuilding the bottle without a revision bump is all that we really need to do.

Why/when do we need to rebuild the bottle in these cases rather than just waiting for the next revision/version to do so?

As of now we have only needed to force a rebuild (not a revision bump) on some specific Linux formulae because they needed to be rebuilt with PIE or had some incorrect caching of some glibc library names. But we seem to be past that mostly - there's only one formula right now that I'm aware of that still needs this rebuild, so this issue may be almost behind us.

The simple approach that I think would work for this is just making a "dummy PR" that only adds a comment, let it run dependent testing, and if nothing breaks then close the PR and just use the dispatch rebottle workflow we already have to rebuild without revision bumping.

I thought we've agreed that we can selectively rebuild bottles like this?

I think the part that was unclear to me was when, if ever, it is acceptable to run the rebottle workflow without first doing the dummy PR for dependent. If the answer to this is "never", that is fine but there didn't seem to be clear documentation on this. As I'm learning more about how this works it seems that maybe all we need is just a PR in our docs that explains when and how to use the rebottle workflow, as it seems to already suit our needs as best as I can tell.

@BrewTestBot
Copy link
Member

:shipit: @danielnachun has triggered a merge.

@MikeMcQuaid
Copy link
Member

As of now we have only needed to force a rebuild (not a revision bump) on some specific Linux formulae because they needed to be rebuilt with PIE or had some incorrect caching of some glibc library names.

In these cases: were the existing bottles broken and, if so, broken by the glibc migration?

I think the part that was unclear to me was when, if ever, it is acceptable to run the rebottle workflow without first doing the dummy PR for dependent.

The rebuild workflow is fine to do but will build for all platforms and drop any bottles for platforms we don't build for any more. As a result, it's not zero cost but is fine to do (and better than a revision).

The build bottle workflow will build a new bottle just for a specific OS. It's preferred whenever possible but requires there to be no existing bottle for this OS. This works well for us on macOS and is what we use for mass rebottling on a new macOS version. I'd love if we could adopt the same process for Linux, too.

@danielnachun danielnachun deleted the djview4 branch October 7, 2022 16:20
@github-actions github-actions bot added the outdated PR was locked due to age label Nov 7, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-bottles Merge without publishing bottles CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. linux/drop-gcc-dependency outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants