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

xplanetFX 2.6.13 #13356

Closed
wants to merge 1 commit into from
Closed

xplanetFX 2.6.13 #13356

wants to merge 1 commit into from

Conversation

blogabe
Copy link

@blogabe blogabe commented May 8, 2017

  • Have you followed the guidelines for contributing?
  • 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?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?
    *** Yes, but it does spit out an error, Dependency pygtk should not use option with-libglade, even though xplanetFX does require libglade with pygtk.

@blogabe
Copy link
Author

blogabe commented May 8, 2017

test-bot failed indicating the above message. Please advise best way to resolve?

@blogabe
Copy link
Author

blogabe commented May 10, 2017

Hi, bumping this up. Please advise given the audit doesn't pass even though xplanetFX requires pygtk to be installed with libglade. Why is this an error in the first place?

@fxcoudert
Copy link
Member

We do not want dependencies to use non-default features anymore.

@blogabe
Copy link
Author

blogabe commented May 11, 2017

So what is the solution in this case as the program requires libglade?

@MikeMcQuaid
Copy link
Member

In this formula's case: we probably remove the GUI option. If it's needed by default: if the option is used by many other packages, we enable it by default. If it's not, we vendor it in the formula with a resource block.

@blogabe
Copy link
Author

blogabe commented May 15, 2017

Thank you @MikeMcQuaid. Unsure why the change to enforce this now however since it's never been an issue before. Certainly the preference is for the GUI option enabled by default. I can't speak to the option being used by many other packages and don't quite get the preferred route here.

@fxcoudert
Copy link
Member

Not having dependencies with non-default options is a new policy. I think the GUI should be dropped, in this case.

PS: xplanetfx is very little used in Homebrew (15 installs a month).

@fxcoudert fxcoudert added needs response Needs a response from the issue/PR author do not merge labels May 18, 2017
@blogabe
Copy link
Author

blogabe commented May 18, 2017

So if I make the GUI diabled by default, this will be fine? Asking because I still fail the audit on my local machine when I try this.

While it's not a heavily used program, it really doesn't make sense to outright drop the GUI option as that will make it impractical to use thereby effectively killing the number of installs.

@BrewTestBot BrewTestBot removed the needs response Needs a response from the issue/PR author label May 18, 2017
@fxcoudert
Copy link
Member

I meant drop the GUI entirely.

@blogabe
Copy link
Author

blogabe commented May 18, 2017

Appreciate that, but that will render it meaningless. I'm checking to see if adding libglade as a GUI dependency and dropping it from pygtk will work. I know that will pass the audit check, but unsure if the program will still work. FWIW, seems heavy handed approach, but I don't have all encompassing view.

@fxcoudert
Copy link
Member

Another option would be to host it in your own personal tap, where you don't have to follow our maintenance rules :)

@blogabe
Copy link
Author

blogabe commented May 18, 2017

Certainly the way it's leaning, unfortunately. Getting this to work by adding libglade separately will not work and must be added with pygtk. I'd prefer to not go through all this for a simply version upgrade.

And running this without the GUI doesn't make sense so it appears my options are running out.

While not having dependencies with non-default options is a new policy, can you shed some light as to why this is a new policy? I'd like to better understand. Thanks

@MikeMcQuaid
Copy link
Member

While not having dependencies with non-default options is a new policy, can you shed some light as to why this is a new policy? I'd like to better understand. Thanks

Basically: this is our (Homebrew's) fault that we didn't add this check long ago. It's lazy packaging, something most other package managers would likely sneer at a bit due to requiring users build things from source unnecessarily at best and have two formulae which demand conflicting requirements of dependencies at worst.

Like a lot of these packaging audits we're introducing: we're never going to intentionally break e.g. dependencies with options but we'll just be making homebrew/core consistently stricter so our users here have a better experience.

@blogabe
Copy link
Author

blogabe commented May 22, 2017

Ok, so re-reading the suggestions here along with reading up on Homebrew/brew#2482 Homebrew/brew#2554 and Homebrew/brew#2627

It doesn't make sense to remove the UI here. I'd prefer to keep this in the homebrew-core rather than fork it. Will "vendoring" make sense here and what exactly does that mean to make everyone happy?

@MikeMcQuaid
Copy link
Member

It doesn't make sense to remove the UI here.

I disagree. In hindsight we should have probably never added the UI here. If you think this makes the formula useless we can consider removing it in favour of your own tap for this.

@blogabe
Copy link
Author

blogabe commented May 25, 2017

Strictly because of the libglade dependency? Seems harsh. Looks more and more like I have to put this on my own tap. I'd like to think about it some more before deciding.

@MikeMcQuaid
Copy link
Member

@blogabe The libglade dependency is the motivator for this but while we're changing it the GUI side is a concern/workaround.

@ilovezfs
Copy link
Contributor

I removed this formula in #14019 primarily due to the wget dependency.

@ilovezfs ilovezfs closed this May 29, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
@blogabe blogabe deleted the xpfx2613 branch January 24, 2019 00:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants