Skip to content
This repository has been archived by the owner on Mar 12, 2019. It is now read-only.

core: Create an X.Org requirement. #10

Closed
wants to merge 1 commit into from
Closed

core: Create an X.Org requirement. #10

wants to merge 1 commit into from

Conversation

rwhogg
Copy link
Contributor

@rwhogg rwhogg commented May 2, 2016

  • 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 ran brew tests with your changes locally?

The existing X11 requirement only works for OS X, and works in concert
with XQuartz. For Linux, we want to install upstream X.Org instead.

@sjackman
Copy link
Member

sjackman commented May 2, 2016

In this case I think it may be more clear to copy and paste the small handful of necessary functions from X11Requirement into XorgRequirement rather than inherit from X11Requirement. We can't really compare the minimum XQuartz version requested by the formula to our X.org version unless we create a fancy mapping table, so the comparison will probably just return that it's always satisfied regardless of the requested version. Oh, but that breaks the brew tests I bet if it always returns satisfied. Hmm.

@sjackman
Copy link
Member

sjackman commented May 2, 2016

Is it possible to not modifyx11_requirement.rb at all?

@rwhogg
Copy link
Contributor Author

rwhogg commented May 2, 2016

Is it possible to not modify x11_requirement.rb at all?

From the looks of it, yes!

@rwhogg
Copy link
Contributor Author

rwhogg commented May 2, 2016

The way I see it, we're trying to depend on the host system as little as possible and the assumption is going to be that linuxbrew/homebrew-xorg/xorg is going to be the canonical supplier of X11. So for all intents and purposes we "know" what the version of X.Org is.

With homebrew/gui, homebrew/x11, and (Linuxbrew's) homebrew/core tapped, there is exactly one formula that (directly) depends on a specific X11 version, and it's gv.

$ ag "depends_on.*x11.*=>.*\""
homebrew-x11/gv.rb
15:  depends_on :x11 => "2.7.2"

@rwhogg
Copy link
Contributor Author

rwhogg commented May 2, 2016

With this in mind, I'm hesitant to introduce any version checking on X.Org. Currently, the version is coming up as "0.0.0", which could be worse, I guess.

We could introduce some version number into it, based on either the upstream release numbers or BLFS', but that would make updating X.Org a pain since it would be in a different repository than this requirement or any tests for it.

@sjackman
Copy link
Member

sjackman commented May 3, 2016

I suggest using the version number of the xorg virtual package, which is the date that it was last updated. The format is currently 20160314, which is fine by me. We could also make it 2016.03.14 to match the typical triple decimal version format. I don't have a real preference.

@sjackman
Copy link
Member

sjackman commented May 3, 2016

With this in mind, I'm hesitant to introduce any version checking on X.Org. …
So for all intents and purposes we "know" what the version of X.Org is.

Yep, I agree. I don't think any version checking is needed.

@@ -39,7 +39,8 @@ def test_dependency_tags

def test_requirement_creation
@d.add :x11
assert_instance_of X11Requirement, find_requirement(X11Requirement)
assert_instance_of X11Requirement, find_requirement(X11Requirement) if OS.mac?
assert_instance_of XorgRequirement, find_requirement(XorgRequirement) unless OS.mac?
Copy link
Member

@sjackman sjackman May 3, 2016

Choose a reason for hiding this comment

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

I'd prefer one of either

if OS.mac?
    assert_instance_of X11Requirement, find_requirement(X11Requirement)
else
    assert_instance_of XorgRequirement, find_requirement(XorgRequirement)
end

or

req = OS.mac? ? X11Requirement : XorgRequirement
assert_instance_of req, find_requirement(req)

with a preference for the latter (untested code). I'm not sure the latter is actually legal code. Guess we'll find out.

@sjackman
Copy link
Member

sjackman commented May 3, 2016

Looking good! I'm glad to see that the tests are passing.

def initialize(name = "xorg", tags = [])
@name = name
if /(\d\.)+\d/ === tags.first
tags.shift
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this means that we chop out the version number for compatibility with X11Requirement, but we don't store it since the XQuartz version numbers are meaningless on Linux.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

@sjackman
Copy link
Member

sjackman commented May 4, 2016

Looking good, Bob! This patch is just about ready to merge.

@sjackman
Copy link
Member

sjackman commented May 5, 2016

👍 Nice! Thanks, Bob.

Should we bottle up X11 before merging this, to give a better user experience and since this pulls in like 70 formula? Are the X11 library bottles cellar :any?

@rwhogg rwhogg added ready and removed in progress labels May 5, 2016
@rwhogg
Copy link
Contributor Author

rwhogg commented May 5, 2016

Should we bottle up X11 before merging this, to give a better user experience and since this pulls in like 70 formula?

Yes, definitely. I'll submit the first batch tonight.

Are the X11 library bottles cellar :any?

Guess we'll find out! My hunch is "no" but we'll see.

@sjackman
Copy link
Member

sjackman commented May 5, 2016

It would be great if they could be cellar :any. They won't be if they look in a fixed location for config files or other resources that is specified at compile-time.

@rwhogg rwhogg added the blocked label May 7, 2016
@rwhogg
Copy link
Contributor Author

rwhogg commented May 7, 2016

Marking as blocked by maxim-belkin/homebrew-xorg#21, but it's a soft-block.

Also note that I'm going to want to sanity-check this one last time before merging.

end

def message
s = "X.Org is required to install this formula."
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest a more descriptive message. Maybe something like something like:
X.Org is required to install this formula. Use: brew tap linuxbrew/xorg && brew install xorg

Copy link
Member

Choose a reason for hiding this comment

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

Since there is a default_formula defined, I believe this message won't ever be displayed. I didn't catch that before. This method def message can be removed entirely.

@rwhogg rwhogg removed the ready label May 22, 2016
The existing X11 requirement only works for OS X, and works in concert
with XQuartz. For Linux, we want to install upstream X.Org instead.
@rwhogg rwhogg closed this in bcb349a May 23, 2016
@rwhogg rwhogg deleted the xorg-requirement branch May 23, 2016 03:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants