Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

Implement Resources #21714

Merged
merged 51 commits into from Sep 12, 2013
Merged

Implement Resources #21714

merged 51 commits into from Sep 12, 2013

Conversation

adamv
Copy link
Contributor

@adamv adamv commented Aug 6, 2013

Making a real pull request for #20212.

resource 'contrib' do
url 'ftp://ftp.figlet.org:21//pub/figlet/fonts/contributed.tar.gz'
sha1 'a23ecfdb54301e93b6578c3c465ba84c8f861d4f'
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When formulae gain revisions, we may want that for resources too; these resources are unversioned tarballs.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are revisions? Is there a roadmap or anything where these future changes are documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@camillol
Copy link
Contributor

camillol commented Aug 7, 2013

Nice! I had a similar idea when working on the Wine formula a while ago, but I assumed you'd reject such a change to the DSL. I would suggest two changes, though:

  1. Handle all downloads using the same mechanism. Patches should use resources too, as should the main formula download. I guess you won't want to have to explicitly declare a "main" resource for each formula, but we can keep the existing DSL and have it create a resource internally.

  2. It should be possible to declare resources inside a devel block, and have them override base formula resources with the same name when the devel block is active. For example, if you look at older versions of the Wine formula you'll see that we had different versions of WineGecko for stable and for devel (and we'll probably have that situation again in the future).

@adamv
Copy link
Contributor Author

adamv commented Aug 7, 2013

(1) I'd love to unify these; Jack has some thoughts on this, but involve rewriting the download strategies (if I remember correctly.)

(2) this is a great idea and solves a design problem I was having; we should allow overrides in head and devel, though it's possible that a --devel build might download stable resources it no longer needs (and them not use them), but I'm OK with that at a first draft.

@jacknagel
Copy link
Contributor

My hope is to move all download and checksum logic into Resource and make SoftwareSpec more Formula-like, so that dependencies can be scoped to stable, devel, or head without using conditionals.

Basically, I want to be able to do most things that are possible in the formula DSL inside of a stable, devel, or head block. Things like depends_on at the top level would retain their current behavior, so, for example, the set of active dependencies for a devel build would be toplevel + those declared in the devel block. Stable-only deps would be inside a separate stable block.

It seems natural that the same semantics would apply to resources.

@adamv
Copy link
Contributor Author

adamv commented Aug 7, 2013

Ready for some review.

@MikeMcQuaid
Copy link
Member

Code looks good to me. I would, in an ideal world, like something where multiple formulae can share a cached resource's download.

@adamv
Copy link
Contributor Author

adamv commented Aug 8, 2013

I was trying (not very well) to articulate something like that in #21716.

@adamv
Copy link
Contributor Author

adamv commented Aug 8, 2013

I'd like for instance to be able to do Formula.factory('python').resource('setuptools')

@adamv
Copy link
Contributor Author

adamv commented Aug 8, 2013

Although for fetch to work, we'd need to be able to declare the resource up-front as a copy from the other formula.

Will think about this.

@adamv
Copy link
Contributor Author

adamv commented Aug 8, 2013

Something like in Python3:

resource 'setuptools' => 'python'
resource 'pip' => 'python'

or

resource 'python' => ['setuptools', 'pip']

@camillol
Copy link
Contributor

camillol commented Aug 8, 2013

@adamv I haven't had time to look at the code; regarding the points 1 and 2 that we were in agreement upon, are they going to be in this PR already, or are they future work?

@adamv
Copy link
Contributor Author

adamv commented Aug 8, 2013

Future work.

@adamv
Copy link
Contributor Author

adamv commented Aug 9, 2013

Do we want sharable resources to be part of this check-in, or are we OK pushing as-is and adding syntax for that later?

@MikeMcQuaid
Copy link
Member

Later seems fine.

@adamv
Copy link
Contributor Author

adamv commented Aug 9, 2013

Can anyone think of a better method name than download here? I'm not entirely happy with it, though it is not terrible.

I kind of want to call it stage.

It should be named with the idea that downloadables will be defined in terms of resources in the future.

@adamv
Copy link
Contributor Author

adamv commented Aug 9, 2013

Some more random API thoughts. Not sold on any of these since none of them happen too often.

Allow resource(name) to take multiple names:

resource('contrib').download(share_fonts)
resource('intl').download(share_fonts)

becomes

resource('contrib', intl').download(share_fonts)

Or, add an extension to Pathname that takes resources:

share_fonts.stage resource('contrib', 'intl')

Though perhaps resource should always return one resource, and a new method resources should return a list of them.

@adamv
Copy link
Contributor Author

adamv commented Aug 9, 2013

Or Pathname.install can learn to handle Resource-like objects too

@adamv
Copy link
Contributor Author

adamv commented Aug 9, 2013

Not enough cases for the resources complexity, removing.

@adamv
Copy link
Contributor Author

adamv commented Aug 9, 2013

Definitely prefer:
man1.stage resource('docs')
to:
resource('docs').download(man1)

@samueljohn
Copy link
Contributor

resource('name').download { <doing stuff here> } looks good, but what is not directly clear is that it also extracts stuff.

Is it also possible to use the resource to just get a file. In homebrew/science, for one formula I need to download lapack.zip and point the install script to that download. How would that look with the resource?

LAPACK_SOURCE = resource('lapack').download ?

@samueljohn
Copy link
Contributor

@adamv Is man1.install resource('docs').download possible?

@camillol
Copy link
Contributor

I like @samueljohn's idea. In fact, there is probably no need for download to take a block - as long as it downloads inside the temp build directory, like patches, it's going to get cleaned up at the end of the brew block. We already have a way of installing files to a destination (man1.install), so it seems that we can get the best composability by simply having resource.download download to a file and return its pathname.

@adamv
Copy link
Contributor Author

adamv commented Aug 11, 2013

We can't download on top of the existing tmp folder, that will overwrite README files or anything with the same name. So the block form is needed.

@camillol
Copy link
Contributor

But if we change the main formula download to be an implicit resource, then we can have a single tmp folder with resource downloads inside - and resource names cannot conflict. Basically instead of /tmp/gj09ghy/ the build directory would be /tmp/gj09ghy/main and other resources would end up in /tmp/gj09ghy/.

@adamv
Copy link
Contributor Author

adamv commented Aug 12, 2013

I taught Pathname.install about resources, and added one example in apktool. Thoughts on this?

(libexec+"lib").install Dir['*.jar']
end
end
(libexec/'lib').stage(resource('bdb-je')) unless build.include? "no-bdb"
Copy link
Contributor

Choose a reason for hiding this comment

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

would (libexec/'lib').install resource('bdb-je') work now, given your latest commit?

I like resource.stage but I don't like libexec.stage resource('foo')

@adamv adamv merged commit 5831a94 into Homebrew:master Sep 12, 2013
@Homebrew Homebrew locked and limited conversation to collaborators Feb 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants