Skip to content
This repository has been archived by the owner. It is now read-only.

0install 2.8 #42405

Closed
wants to merge 1 commit into from
Closed

0install 2.8 #42405

wants to merge 1 commit into from

Conversation

@afb
Copy link
Contributor

@afb afb commented Aug 3, 2015

Another attempt, after the previous formula had several issues. Hopefully this is better...


depends_on :gpg
depends_on "curl"
depends_on "gtk+" if build.with? "gui"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 3, 2015
Member

Use => :optional here instead, thanks.

This comment has been minimized.

@afb

afb Aug 3, 2015
Author Contributor

How do I translate the :optional/:recommended over to the build option of using lablgtk or not ?
Should the GUI just be made mandatory, so that there is no way to install CLI only (skip gtk+)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 3, 2015
Member

The option will be named without-gtk+ instead. It should also probably default to off.

This comment has been minimized.

@afb

afb Aug 3, 2015
Author Contributor

If I default it to off, it won't be in the bottle ? That was the main reason for leaving it on.
In other distributions, the GUI goes in a separate package: "0install" vs. "0install-core"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 3, 2015
Member

Yeh, we default to more minimal versions.

This comment has been minimized.

@afb

afb Aug 3, 2015
Author Contributor

That is unfortunate (no gui bottle), since gtk+ is one of the more annoying ones.
But at least you can install that + deps (and ocaml/opam + boost) from bottles...

depends_on "opam" => :build

if build.head?
# opam install yojson xmlm ounit react lwt extlib ocurl lablgtk sha

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 3, 2015
Member

Remove this comment and use a head do block.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 3, 2015
Member

Oh, also, these should just be resources instead or remove the head block.

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Aug 3, 2015

Maybe worth just having the non-head build in the formula first? Looks like the head build is quite a bit more complicated?

@afb
Copy link
Contributor Author

@afb afb commented Aug 3, 2015

Actually it is the non-head build that is complicated, due to the want of using resources...
The current build is as simple as (or it would be, if wasn't for the keg-only dependencies):

$ brew install pkg-config gettext gnupg gtk+ objective-caml opam
$ opam install yojson xmlm ounit react lwt extlib ocurl sha lablgtk
$ make
$ sudo make install

But that doesn't yield us any bottles, which means that users need to install ocaml/opam.
So as per the cookbook, the ocaml modules are locally vendored instead of dependencies.

Resources need to be versioned, however, so then it doesn't work with a HEAD build ?
But I can remove the head sections, and convert the "gui" flag over to :recommended...

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Aug 3, 2015

Resources could potentially be unversioned Git repositories if they are only in the head do block.

@afb
Copy link
Contributor Author

@afb afb commented Aug 3, 2015

@MikeMcQuaid: the resources are all in the same repository, but I suppose that could be done too...

@afb afb force-pushed the afb:0install branch from 7f10ba5 to 6428fa7 Aug 3, 2015
@afb afb force-pushed the afb:0install branch from 6428fa7 to fd51043 Aug 4, 2015
url "https://opam.ocaml.org/archives/lablgtk.2.18.3+opam.tar.gz"
version "2.18.3"
sha256 "f0b7ed0bd85f6cf4b4c5f81966f03763e76bb9f866f5172511ce48cf31fd433c"
end if build.with? "gtk+"

This comment has been minimized.

@DomT4

DomT4 Aug 14, 2015
Member

This should be:

if build.with? "gtk"
  resource "lablgtk" do
    url "https://opam.ocaml.org/archives/lablgtk.2.18.3+opam.tar.gz"
    version "2.18.3"
    sha256 "f0b7ed0bd85f6cf4b4c5f81966f03763e76bb9f866f5172511ce48cf31fd433c"
  end
end

The end if lines are confusing and we've been trying to wipe them out for a while.

# Parallel builds fail for some of these opam libs.
ENV.deparallelize

opamroot=buildpath/"opamroot"

This comment has been minimized.

@DomT4

DomT4 Aug 14, 2015
Member

Should be spacing here. opamroot = buildpath/"opamroot"

opamroot=buildpath/"opamroot"
ENV["OPAMROOT"] = opamroot
ENV["OPAMYES"] = "1"
mkdir_p opamroot/"archives"

This comment has been minimized.

@DomT4

DomT4 Aug 14, 2015
Member

(opamroot/"archives").mkpath looks a little nicer.

system "opam", "init", "--no-setup"
modules = []
resources.each do |r|
r.verify_download_integrity(r.fetch)

This comment has been minimized.

@DomT4

DomT4 Aug 14, 2015
Member

Is this doing checksums? Why wouldn't that checksum happen when we fetch the resources?

This comment has been minimized.

@afb

afb Aug 15, 2015
Author Contributor

TBH it was just copied from the opam formula, seems that is indeed doing checksumming twice

This comment has been minimized.

@afb

afb Aug 15, 2015
Author Contributor

Looks like it was copied from Resource.stage:

  def stage(target = nil, &block)
    unless target || block
      raise ArgumentError, "target directory or block is required"
    end

    verify_download_integrity(fetch)
    unpack(target, &block)
  end
@afb afb force-pushed the afb:0install branch from fd51043 to 05c2421 Aug 15, 2015
@afb
Copy link
Contributor Author

@afb afb commented Aug 15, 2015

Hope this isn't another one of those nit-pickings where just it ends up lying around in the end, like #28771

@afb afb force-pushed the afb:0install branch from 05c2421 to 3e731e2 Aug 15, 2015
@DomT4
Copy link
Member

@DomT4 DomT4 commented Aug 15, 2015

Referencing a PR that was some 15000 Issues/PRs back isn't terribly helpful. Maintainers want things to be as expected before going in otherwise at some point we end up having to fix them. If it meets maintainer's concerns & our minimum addition requirements, it will be merged.

As far as I can see, there's not much left to deal with before this is fine. Three nits, and we should be golden.

sha256 "12de771be748bce9350c90bc4720029a566b078ceabd335af09386ac6a37df2b"

depends_on :gpg
depends_on "curl"

This comment has been minimized.

@DomT4

DomT4 Aug 15, 2015
Member

Is it incapable of using the system curl?

This comment has been minimized.

@afb

afb Aug 15, 2015
Author Contributor

Nope, it should be "new enough". Dropping the curl then...

depends_on "gtk+" => :optional
depends_on "pkg-config" => :build
depends_on "ocaml" => :build
depends_on "opam" => :build

This comment has been minimized.

@DomT4

DomT4 Aug 15, 2015
Member

It's not really that important, but for future referencing sake, we prefer something like:

depends_on "" => :build
depends_on "" => :build
depends_on ""
depends_on "" => :optional
depends_on :special_dep

Build deps, then mandatory deps, then optional deps, then special deps.

This comment has been minimized.

@afb

afb Aug 15, 2015
Author Contributor

Hmm, don't think neither "audit" or "style" said anything about this...

This comment has been minimized.

@DomT4

DomT4 Aug 15, 2015
Member

It won't. We don't codify it, yet.

</implementation>
</interface>
EOS
assert_equal "hello world\n", `#{bin}/0launch --console hello.xml`

This comment has been minimized.

@DomT4

DomT4 Aug 15, 2015
Member

Can this use shell_output("#{bin}/0launch --console hello.xml") instead of backtics?

This comment has been minimized.

@afb

afb Aug 15, 2015
Author Contributor

Most likely. I didn't write it. Is it better/safer with that function than backticks ?

This comment has been minimized.

@DomT4

DomT4 Aug 15, 2015
Member

We prefer it over backtics inside the test do blocks, aye.

@afb afb force-pushed the afb:0install branch from 3e731e2 to bec819c Aug 15, 2015
@afb
Copy link
Contributor Author

@afb afb commented Aug 15, 2015

@DomT4: ok, thanks.

seems like a missed one new dep-of-a-dep, and put the archives in the wrong place...
so pushed a new commit with that fixed. was trying to get gtk+ to build just the optional plugin,
but it seems that it doesn't work as a "build" dependency (doesn't pull in the deps like pango)
so the bottle will default to console-only, and needs a formula rebuild for the gui to work.

@afb afb force-pushed the afb:0install branch from bec819c to 20b6669 Aug 15, 2015
@afb
Copy link
Contributor Author

@afb afb commented Aug 15, 2015

@DomT4: seems like it doesn't download the resources on just brew install (not sure when, "fetch" ?)
so if you change a dependency it looks like it can go missing, just got a file-not-found at least.
trying to upgrade the formula to the later version (2.9), but also want to upgrade the ocaml modules...
and if there was a way to get the "gui_gtk.cmxs" (plugin bundle) to build, that would be great too.

@afb
Copy link
Contributor Author

@afb afb commented Aug 15, 2015

i.e. the :build doesn't work, because it only adds "gtk+" to the path but not "pango" and "glib" etc etc.
tried with ENV.append_path "PKG_CONFIG_PATH", "#{lib}/pkgconfig", but that hack didn't work either.

@afb
Copy link
Contributor Author

@afb afb commented Aug 15, 2015

ah, normally the stage is doing the fetch and verify. so need to add that line back to the formula.
(these resources are unpacked by opam, better let it handle it and get the suffix it prefers etc etc)

@afb afb force-pushed the afb:0install branch from 20b6669 to 3b74517 Aug 15, 2015
@DomT4
Copy link
Member

@DomT4 DomT4 commented Aug 15, 2015

Funky. As long as it works, I'm happy to revert to the opam way mimicked previously.

@afb
Copy link
Contributor Author

@afb afb commented Aug 15, 2015

It was pointed out to me that build deps having broken pkg-config is a feature: #41759
So that probably means that we cannot build the gui in the bottle, but only conditionally

@DomT4
Copy link
Member

@DomT4 DomT4 commented Aug 15, 2015

As long as it works optionally that's fine. We very rarely build GUIs by default anyhow.

Sent from my iPhone

On 15 Aug 2015, at 19:22, Anders F Björklund notifications@github.com wrote:

It was pointed out to me that build deps having broken pkg-config is a feature: #41759
So that probably means that we cannot build the gui in the bottle, but only conditionally


Reply to this email directly or view it on GitHub.


def install
# Parallel builds fail for some of these opam libs.
ENV.deparallelize

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Aug 26, 2015
Member

Can you report issues upstream for these? Thanks!

This comment has been minimized.

@afb

afb Aug 26, 2015
Author Contributor

Will do (as soon as I remember which of the modules it was that failed)
Hmm, build worked fine without it. Maybe the -j1 caution got outdated.

@afb afb force-pushed the afb:0install branch from 3b74517 to 831eb3d Aug 26, 2015
@talex5
Copy link

@talex5 talex5 commented Oct 7, 2015

What else needs to happen for this to get merged?

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Oct 13, 2015

Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants