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

bintray: fix package creation #7415

Merged
merged 1 commit into from Apr 21, 2020

Conversation

bayandin
Copy link
Member

@bayandin bayandin commented Apr 21, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR fixes bintray package creation.

Also, it changes ::package_exists? to return boolean value instead of SystemCommand instance

Ref: Homebrew/homebrew-core#53304 (comment)

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When did this break? Was it a particular PR or a change on Bintray's/our side? Thanks!

Comment on lines +71 to +72
stderr = e.output.select { |type,| type == :stderr }
.map { |_, line| line }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stderr = e.output.select { |type,| type == :stderr }
.map { |_, line| line }
stderr = e.output
.select { |type,| type == :stderr }
.map { |_, line| line }

or, if it works:

Suggested change
stderr = e.output.select { |type,| type == :stderr }
.map { |_, line| line }
stderr = e.output
.select { |type,| type == :stderr }
.values

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second option won't work because .select { |type,| type == :stderr } returns an array.
Will create a PR with the first one

@bayandin
Copy link
Member Author

When did this break? Was it a particular PR or a change on Bintray's/our side? Thanks!

I think we have never used API for package creation since it was added in #7236

Line 116 always raises an exception because package is undefined:

if !formula_packaged[formula_name] && !package_exists?(repo: bintray_repo, package: bintray_package)
odebug "Creating package #{@bintray_org}/#{bintray_repo}/#{package}"
create_package repo: bintray_repo, package: bintray_package
formula_packaged[formula_name] = true
end

@MikeMcQuaid
Copy link
Member

I think we have never used API for package creation since it was added in #7236

Gotcha so basically: no-one has used brew pr-pull until now?

@bayandin
Copy link
Member Author

bayandin commented Apr 21, 2020

no-one has used brew pr-pull until now?

No-one has used brew pr-pull for a new formula

@MikeMcQuaid MikeMcQuaid merged commit 20f7f26 into Homebrew:master Apr 21, 2020
@MikeMcQuaid
Copy link
Member

Thanks @bayandin!

@bayandin
Copy link
Member Author

Thanks @MikeMcQuaid!

But I didn't have a chance to apply your suggestions 🙄

@Bo98
Copy link
Member

Bo98 commented Apr 21, 2020

No-one has used brew pr-pull for a new formula

People have, but it turns out it doesn't fail pr-publish (which invokes pr-pull internally in the workflow) when it happens.

Volta worked fine (done via pr-pull directly - not pr-publish), but likely ran at least once through the Homebrew Bottles Jenkins job which will have created the package on Bintray.

@bayandin bayandin deleted the fix-pushing-bottles branch April 21, 2020 14:10
@MikeMcQuaid
Copy link
Member

But I didn't have a chance to apply your suggestions 🙄

Oops, sorry, misread this. What do you think of them?

@bayandin bayandin mentioned this pull request Apr 22, 2020
6 tasks
@Bo98
Copy link
Member

Bo98 commented Apr 25, 2020

Interestingly, this doesn't work in the CI: https://github.com/Homebrew/homebrew-core/runs/618091933?check_suite_focus=true

Yet, I run pr-pull locally and it works fine.

Judging by the "Linuxbrew/2.2.13-138-gd5ab653", it seems to be the latest version so isn't an update issue.

@bayandin
Copy link
Member Author

Hmm, interesting.

Can it be related to the curl version that we use? It seems it is 7.47.0 on the agent (took it from UA string)

@Bo98
Copy link
Member

Bo98 commented Apr 25, 2020

Ok so it fails to properly check if a package exists when you have the --verbose flag.

The logic here relies on --fail being passed, but that doesn't happen when show_output is true, which is the case for --verbose.

--fail being tied with show_output seems a bit weird to me.

@bayandin
Copy link
Member Author

Yup, have found it as well, as was surprised too 😄

#7435 will fix it for now

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 2, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants