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

dev-php/composer-1.2.2 add ebuild #2905

Conversation

GuillaumeSeren
Copy link
Contributor

@GuillaumeSeren GuillaumeSeren commented Nov 24, 2016

Hey,
these are the ebuilds for composer-1.2.2, and it's dependencies,
here the list:
dev-php/fedora-autoloader-0.2.1
dev-php/fig-log-1.0.2
dev-php/symfony-process-2.8.12
dev-php/symfony-filesystem-2.7.20
dev-php/symfony-finder-2.7.20
dev-php/phar-utils-1.0.0
dev-php/cli-prompt-1.0.0
dev-php/json-schema-2.0.0
dev-php/jsonlint-1.4.0
dev-php/spdx-licenses-1.0.0
dev-php/semver-1.0.0
dev-php/ca-bundle-1.0.0
dev-php/symfony-config-2.1.0
dev-php/symfony-yaml-2.1.0
dev-php/symfony-dependency-injection-2.1.0
dev-php/symfony-event-dispatcher-2.1.0
dev-php/symfony-console-2.7.9
dev-php/composer-1.2.2

Those ebuilds use dev-php/fedora-autoloader to provide a PSR4 autoloader,
picking each dependencies name and version are done from the composer.jon of each project.

I setup IUSE='test' when the test suite worked or existed (6 packages), but for most of them the test did not work well, and so I did not activate the flag at all.

I was helped by the information on the gentoo BTS here (https://bugs.gentoo.org/show_bug.cgi?id=439206), this wiki page (https://wiki.gentoo.org/wiki/Project:PHP/The_Composer_problem) and finally the fedora work on composer (thanks to them) here (http://pkgs.fedoraproject.org/cgit/rpms/composer.git/) and (https://blog.remirepo.net/post/2015/06/30/PHP-SIG-autoloader), and Michael Orlitzky.

I've made a commit for each separate package, and ordered them by dependency order.

Guillaume

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: dev-php/ca-bundle, dev-php/cli-prompt, dev-php/composer, dev-php/fedora-autoloader, dev-php/fig-log...

dev-php/ca-bundle: @gentoo/proxy-maint (new package)
dev-php/cli-prompt: @gentoo/proxy-maint (new package)
dev-php/composer: @gentoo/proxy-maint (new package)
dev-php/fedora-autoloader: @gentoo/proxy-maint (new package)
dev-php/fig-log: @gentoo/proxy-maint (new package)
dev-php/json-schema: @gentoo/proxy-maint (new package)
dev-php/jsonlint: @gentoo/proxy-maint (new package)
dev-php/phar-utils: @gentoo/proxy-maint (new package)
dev-php/semver: @gentoo/proxy-maint (new package)
dev-php/spdx-licenses: @gentoo/proxy-maint (new package)
dev-php/symfony-config: @gentoo/proxy-maint (new package)
dev-php/symfony-console: @gentoo/proxy-maint (new package)
dev-php/symfony-dependency-injection: @gentoo/proxy-maint (new package)
dev-php/symfony-event-dispatcher: @gentoo/proxy-maint (new package)
dev-php/symfony-filesystem: @gentoo/proxy-maint (new package)
dev-php/symfony-finder: @gentoo/proxy-maint (new package)
dev-php/symfony-process: @gentoo/proxy-maint (new package)
dev-php/symfony-yaml: @gentoo/proxy-maint (new package)

@gentoo-repo-qa-bot gentoo-repo-qa-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). labels Nov 24, 2016
@mgorny
Copy link
Member

mgorny commented Nov 24, 2016

@gentoo/php, could someone take a look at this, please? ;-)

@orlitzky
Copy link
Contributor

I mean to take a good long look at all of these, but I've had an insane week and it's not over yet. Here are some more things to work on in the meantime =)

Have you talked to the proxy-maint team about maintaining these packages (since you're listed in the metadata.xml files)? That's fine with me, but they have a bunch of "paperwork" these days that they like to fill out when a user begins to maintain packages in the tree. If not, you should be able to ping them in #gentoo-proxy-maint on freenode.

  • The S variable assignment should be quoted, because WORKDIR can contain spaces.
  • Comments like "#Install the doc" right before a dodoc are a bit redundant.
  • The fedora-autoloader dependency should be a runtime dependency instead of build-time.
  • Specifying =dev-php/fedora-autoloader-0.2.1 is probably too restrictive... for now, simply dev-php/fedora-autoloader will work.
  • Please delete the src_test phases that are entirely commented out, and the commented IUSE=test lines too.
  • I think you can delete src_prepare from spdx-licenses-1.0.0.ebuild and composer-1.2.2.ebuild entirely.
  • The tests that bootstrap with a file in /usr aren't testing the version that's being installed. Instead, they're testing the version that's already installed (in /usr). In particular, the tests will fail the first time you install the package, but in general it will test the version that's about to be replaced.
  • The symfony packages that require one another should do so in RDEPEND instead of DEPEND. They can optionally be added to DEPEND as well if you need them; for example, to run the tests at build-time.

It's getting close, thanks again for all your hard work.

@GuillaumeSeren
Copy link
Contributor Author

GuillaumeSeren commented Nov 25, 2016

Hi Michael,
thank for the feedback and support !

Have you talked to the proxy-maint team about maintaining these packages (since you're listed in the metadata.xml files)? That's fine with me, but they have a bunch of "paperwork" these days that they like to fill out when a user begins to maintain packages in the tree. If not, you should be able to ping them in #gentoo-proxy-maint on freenode.

I have gone there and post my request, I am waiting for the reply.

The tests that bootstrap with a file in /usr aren't testing the version that's being installed. Instead, they're testing the version that's already installed (in /usr). In particular, the tests will fail the first time you install the package, but in general it will test the version that's about to be replaced.

Yes I have noticed it, so you need to install it first and the test only run against the installed version wihich might not be intended (if you try to test a different version for example).
To really fix that we could inject, before src_test (not sure how it is possible), a local autoloader that target the needed libraries but again they had to be installed first (with the right version).

On the test subject, there many test-suite that break on some tests or just skip some test, that are not for linux platform, here is an example of 'dev-php/symfony-process':

phpunit --bootstrap /usr/share/php/Symfony/Process/autoload.php --verbose
PHPUnit 4.3.1 by Sebastian Bergmann.

Configuration read from /home/gseren/composer-gentoo/process-2.8.12/phpunit.xml.dist

....SS.S.S.........................S...........................  63 / 151 ( 41%)
.................S.S.............................SS............ 126 / 151 ( 83%)
.........................

Time: 3.72 seconds, Memory: 19.25Mb

There were 9 skipped tests:

1) Symfony\Component\Process\Tests\ExecutableFinderTest::testFindProcessInOpenBasedir
Cannot test when open_basedir is set

/home/gseren/composer-gentoo/process-2.8.12/Tests/ExecutableFinderTest.php:116

2) Symfony\Component\Process\Tests\PhpExecutableFinderTest::testFindWithPhpPath
The PHP binary is easily available as of PHP 5.4

/home/gseren/composer-gentoo/process-2.8.12/Tests/PhpExecutableFinderTest.php:27

3) Symfony\Component\Process\Tests\PhpExecutableFinderTest::testFindWithHHVM
Should be executed in HHVM context.

/home/gseren/composer-gentoo/process-2.8.12/Tests/PhpExecutableFinderTest.php:71

4) Symfony\Component\Process\Tests\PhpExecutableFinderTest::testFindWithSuffix
The PHP binary is easily available as of PHP 5.4

/home/gseren/composer-gentoo/process-2.8.12/Tests/PhpExecutableFinderTest.php:104

5) Symfony\Component\Process\Tests\ProcessTest::testStopWithTimeoutIsActuallyWorking
Extension pcntl is required.

6) Symfony\Component\Process\Tests\ProcessTest::testTTYInWindowsEnvironment
This test is for Windows platform only

/home/gseren/composer-gentoo/process-2.8.12/Tests/ProcessTest.php:477

7) Symfony\Component\Process\Tests\ProcessTest::testPTYCommand
PTY is not supported on this operating system.

/home/gseren/composer-gentoo/process-2.8.12/Tests/ProcessTest.php:496

8) Symfony\Component\Process\Tests\ProcessTest::testSignal
Extension pcntl is required.

9) Symfony\Component\Process\Tests\ProcessTest::testExitCodeIsAvailableAfterSignal
Extension pcntl is required.
                                            
OK, but incomplete, skipped, or risky tests!
Tests: 151, Assertions: 245, Skipped: 9. 

I will do the modifications you asked asap.

@GuillaumeSeren
Copy link
Contributor Author

GuillaumeSeren commented Nov 25, 2016

I have done the proxy-maintainer bug opening here: https://bugs.gentoo.org/show_bug.cgi?id=600800

GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 25, 2016
GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 26, 2016
@GuillaumeSeren GuillaumeSeren force-pushed the dev-php/composer-1.2.2-add-ebuild branch from c5093d4 to 2832c9f Compare November 26, 2016 18:07
@GuillaumeSeren
Copy link
Contributor Author

Hey,
I work on the ebuilds and add all (I think) feedback to them, I just tested reinstall composer, and all the dependencies it still work, here a summary of what I have done:

  • Clean all useless comments.
  • Move all dependencies to RDEPEND (from DEPEND), as they're just needed for running it, and it will clean / simpler to add the real DEPEND for test purpose for example.
  • Cleanup src_prepare.
  • Rebase all the 18 commits to maintain each package distinct.

Guillaume

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

Please see the comments below. All of them apply to multiple packages. I'm sorry but it'll take a bit of work to get it up to our standards.

Furthermore, please update the commit messages to suit our standards. https://wiki.gentoo.org/wiki/Gentoo_git_workflow has some tips.

Generally, the idea is to include only minimal information related to the commit there, in git-friendly format. Include bug no if you have bugs for (some of) those packages. Don't include URLs or dependency lists (one can read them from ebuilds). Reason for introduction is a good idea though.


DESCRIPTION="Find a path to the system CA bundle, and fallback to the Mozilla CA bundle"
HOMEPAGE="https://github.com/composer/ca-bundle"

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why you add this empty line? We generally try to preserve the order and grouping variables from the devmanual, to help readability, and this bites me in the eyes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason just grouping the upstream url and description together, I'll fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone on IRC point me at the skel ebuild (https://gitweb.gentoo.org/repo/gentoo.git/tree/skel.ebuild) and there is quite some blank line there, nearly after each definition of variable,
I delete the blank line as you suggested but I am a bit confuse about that.

Copy link
Member

Choose a reason for hiding this comment

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

.skel uses blanks to make the comments more readable. I think the author meant to use double blank lines where single blank line is intended in ebuild but somehow failed that around EAPI/inherit. Here's a reference: https://devmanual.gentoo.org/quickstart/index.html

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Nov 28, 2016

Choose a reason for hiding this comment

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

Yes I understand it will be done in the next version,
I'll bookmark your devmanual link thank you

<email>guillaumeseren@gmail.com</email>
<name>Guillaume Seren</name>
</maintainer>
<longdescription>
Copy link
Member

Choose a reason for hiding this comment

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

Please also add the proxy-maint project here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done

@@ -0,0 +1,4 @@
AUX autoload.php 268 SHA256 e4432f30b3120969792ca0af5544cbd68ca1d384a997f92f5d6424d9677a17c1 SHA512 186ccfc3ae6669722d01948c266a9c0518e1da228095a880695b7de6502c9798203019622cc0c63e30fbf7abe94aa988b7c694bcc98522e04298aa90217b8ed8 WHIRLPOOL 6632ff7476390853c5f4d93bbb58c82d6c7c8e0efdb81b93ae58dfe5e104a4e4fb248203a0ead62a23cb5e64d0723bb5ed11b3f88c874fb6e1e42423424a73e1
Copy link
Member

Choose a reason for hiding this comment

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

Please regenerate those Manifests inside git repository checkout. They should not contain anything but DIST entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I've generated them in my local overlay, and I am not sure if it is different to generate them from inside the git but I will do it after I finish, all the mods.
Is the the repoman command the same ? (repoman manifest)

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll do it, in the case of my overlay, I should regenerate manifest as well or I can keep the local one ? (just curious I have noticed a difference before)

Copy link
Member

Choose a reason for hiding this comment

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

That depends on your metadata/layout.conf, it's described in man 5 portage. thin-manifests = true -- Manifests contain only DIST, other entries are irrelevant. = false -- all files are included in Manifests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes it has been upgraded to the page I'd read to do my local overlay: https://wiki.gentoo.org/wiki/Overlay/Local_overlay

So I am upgrading my confs, thank for the reminder!

KEYWORDS="~amd64 ~x86"

RDEPEND="dev-lang/php:*
dev-lang/php[curl]
Copy link
Member

Choose a reason for hiding this comment

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

Please merge this into the php:* atom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done

RDEPEND="dev-lang/php:*
dev-lang/php[curl]
dev-php/fedora-autoloader
=dev-php/json-schema-2.0.0
Copy link
Member

Choose a reason for hiding this comment

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

= deps are almost always a very bad idea. Please at least use ~ to account for revision bumps.

src_install() {
# I've kept the same path name that Fedora use
insinto "/usr/share/php/Composer/Composer"
doins -r src/Composer/*
Copy link
Member

Choose a reason for hiding this comment

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

As a nice trick, you could use . instead of * to avoid in-place filename expansion. This makes it a bit faster, a bit cleaner and ensures that dotfiles are installed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok it will be done in the next version

# I've kept the same path name that Fedora use
insinto "/usr/share/php/Composer/Composer"
doins -r src/Composer/*
doins -r res
Copy link
Member

Choose a reason for hiding this comment

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

You can pass multiple filenames (paths) to dodoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i'll upgrade that

}

-require __DIR__.'/../src/bootstrap.php';
+require '/usr/share/php/Composer/Composer/autoload.php';
Copy link
Member

Choose a reason for hiding this comment

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

Could you merge those four patches into one? They all pretty much serve the same purpose — updating paths for Gentoo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it will be done in the next version

DESCRIPTION="Fedora's Static PSR-4, PSR-0, and classmap autoloader"
HOMEPAGE="https://github.com/php-fedora/autoloader"

# here we use autoloadder as it is the upstream name
Copy link
Member

Choose a reason for hiding this comment

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

You've got typo. Besides, I don't think the comment is really necessary. It's kinda obvious that SRC_URI uses upstream name, and having an explicit comment saying that makes me wonder it has some extra meaning — i.e. in the end, it causes only more confusion.

Is there any reason why you don't install it as dev-php/autoloader btw? You seem to be inconsistenly using bare package name for some packages, and vendor+package for some.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a choice I made with the help of @orlitzky 2 arguments for that:

  • stick with the fedora name to avoid naming issue.
  • stick to the namespace, that's the thing I've done in all other case, that's help finding the dependencies.
  • extra it is a reference to fedora t have started the project which helped a lot doing that.

So I dropped the comment line, but keep the name, are you ok @mgorny ?

Copy link
Member

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

}

src_test() {
phpunit --bootstrap /usr/share/php/Symfony/DependencyInjection/autoload.php || die "test suite failed"
Copy link
Member

Choose a reason for hiding this comment

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

Unless I'm missing something, you're passing path to the installed package to src_test() that is run before it's installed. It should reference some kind of ${S} path instead.

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Nov 27, 2016

Choose a reason for hiding this comment

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

I am not sure about that one,
Yes it should call the work environment source and not the installed one, but we have 2 cases to consider:

  • No dependency related, we could change to load the --bootstrap , but that also mean change the autoloader content to be relevant in that context, and so breaking it in the installed context, we could want to duplicate it just for the test.
  • When there is multiple libs intended to be loaded the problem become, that we should require them in DEPEND to be installed before, and cause duplicate, but can be done.

Not really sure what choice is better, but the actual situation is the test works only if the package is already installed, I agree this is not good enough, so IMO we could have a test-autoloader for that.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand your dependency problem. All the relevant libraries are in DEPEND already anyway, aren't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They was but I moved them to RDEPEND, as it is only needed for runtime and also suggested by orlitzky feedback (as I understand it, I may been wrong), but anyway it didn't really change the duplication of the autoloader issue right ?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, in that case you usually want some cheap trick along the lines of:

RDEPEND="..."
DEPEND="test? ( ${RDEPEND} )"

(and 'test' in IUSE appropriately)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that works as well. The variant with test? is used if you don't need them at build them otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will change that for the next upgrade by default package with no test will have DEPEND empty, and use the test? for the one with working tests.

I have to implement the test autoloader but I have move forward on the rest on your feedback @mgorny so I will work on that asap (the partial upgrade are on my overlay).

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Nov 29, 2016

Choose a reason for hiding this comment

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

@mgorny I have checked the content of the workdir to find the autoloader, before making a new one specific for tests, so I am not sure where to find it !

/var/tmp/portage/dev-php/jsonlint-1.4.0/
├── build-info
│   └── jsonlint-1.4.0.ebuild
├── distdir
│   └── jsonlint-1.4.0.tar.gz -> /usr/portage/distfiles/jsonlint-1.4.0.tar.gz
├── homedir
├── temp
│   ├── build.log
│   ├── environment
│   └── logging
└── work
    └── jsonlint-1.4.0
        ├── bin
        │   └── jsonlint
        ├── CHANGELOG.mdown
        ├── composer.json
        ├── LICENSE
        ├── phpunit.xml.dist
        ├── README.mdown
        ├── src
        │   └── Seld
        │       └── JsonLint
        │           ├── DuplicateKeyException.php
        │           ├── JsonParser.php
        │           ├── Lexer.php
        │           ├── ParsingException.php
        │           └── Undefined.php
        └── tests
            ├── bom.json
            ├── bootstrap.php
            └── JsonParserTest.php

12 directories, 18 files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I finally obtain a 'autoloader-test.php', like that

<?php
/* Autoloader for composer/ca-bundle and its dependencies */

if (!class_exists('Fedora\\Autoloader\\Autoload', false)) {
    require_once '/usr/share/php/Fedora/Autoloader/autoload.php';
}

\Fedora\Autoloader\Autoload::addPsr4('Seld\\JsonLint\\', '/var/tmp/portage/dev-php/jsonlint-1.4.0/work/jsonlint-1.4.0/src/Seld/JsonLint/');

It is a bit hard-coded to my taste so I could add a sed call in the ebuild to make a bit more dynamic, but it works with the lib not installed I think it is the good path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I did it a cleaner than what I said, I added a src_prepare to copy the autoloader file from the FILESDIR in the WORKDIR, like

RDEPEND="dev-lang/php:*
        =dev-php/fedora-autoloader-0.2.1"
DEPEND="test? ( ${RDEPEND}
                                dev-php/phpunit )"

src_prepare() {
        default
        if use test; then
                cp "${FILESDIR}"/autoload.php "${S}"/autoload-test.php
                sed -i -e "s:__DIR__:'${S}/src/Seld/JsonLint':" "${S}"/autoload-test.php
        fi
}

src_install() {
        insinto "/usr/share/php/Seld/JsonLint"
        doins -r src/Seld/JsonLint/. "${FILESDIR}"/autoload.php
        dodoc README.mdown
}

src_test() {
        phpunit --bootstrap "${S}"/autoload-test.php || die "test suite failed"
}

After I change the --bootstrap to target this new file, so now the test works when not installed,
so I can install the composer with FEATURES=test and it pull everything and tests works

FEATURES=test emerge -av "=dev-php/composer-1.2.2"
 * IMPORTANT: 3 news items need reading for repository 'gentoo'.
 * Use eselect news read to view new items.


These are the packages that would be merged, in order:

Calculating dependencies... done!
[ebuild  N     ] dev-php/phpunit-4.3.1::gentoo  0 KiB
[ebuild  N    #] dev-php/fedora-autoloader-0.2.1::GuillaumeSeren  USE="{test}" 0 KiB
[ebuild  N    #] dev-php/fig-log-1.0.2::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/symfony-process-2.8.12::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/semver-1.0.0::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/spdx-licenses-1.0.0::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/symfony-finder-2.7.20::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/jsonlint-1.4.0::GuillaumeSeren  USE="{test}" 0 KiB
[ebuild  N    #] dev-php/symfony-filesystem-2.7.20::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/json-schema-2.0.0::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/phar-utils-1.0.0::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/ca-bundle-1.0.0::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/cli-prompt-1.0.0::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/symfony-yaml-2.1.0::GuillaumeSeren  USE="{test}" 0 KiB
[ebuild  N    #] dev-php/symfony-config-2.1.0::GuillaumeSeren  USE="{test}" 0 KiB
[ebuild  N    #] dev-php/symfony-dependency-injection-2.1.0::GuillaumeSeren  USE="{test}" 0 KiB
[ebuild  N    #] dev-php/symfony-event-dispatcher-2.1.0::GuillaumeSeren  USE="{test}" 0 KiB
[ebuild  N    #] dev-php/symfony-console-2.7.9::GuillaumeSeren  0 KiB
[ebuild  N    #] dev-php/composer-1.2.2::GuillaumeSeren  0 KiB

Total: 19 packages (19 new), Size of downloads: 0 KiB

Would you like to merge these packages? [Yes/No] Yes
>>> Verifying ebuild manifests
>>> Emerging (1 of 19) dev-php/phpunit-4.3.1::gentoo
>>> Installing (1 of 19) dev-php/phpunit-4.3.1::gentoo
>>> Emerging (2 of 19) dev-php/fedora-autoloader-0.2.1::GuillaumeSeren
>>> Installing (2 of 19) dev-php/fedora-autoloader-0.2.1::GuillaumeSeren
>>> Emerging (3 of 19) dev-php/fig-log-1.0.2::GuillaumeSeren
>>> Installing (3 of 19) dev-php/fig-log-1.0.2::GuillaumeSeren
>>> Emerging (4 of 19) dev-php/symfony-process-2.8.12::GuillaumeSeren
>>> Installing (4 of 19) dev-php/symfony-process-2.8.12::GuillaumeSeren
>>> Emerging (5 of 19) dev-php/semver-1.0.0::GuillaumeSeren
>>> Installing (5 of 19) dev-php/semver-1.0.0::GuillaumeSeren
>>> Emerging (6 of 19) dev-php/spdx-licenses-1.0.0::GuillaumeSeren
>>> Installing (6 of 19) dev-php/spdx-licenses-1.0.0::GuillaumeSeren
>>> Emerging (7 of 19) dev-php/symfony-finder-2.7.20::GuillaumeSeren
>>> Installing (7 of 19) dev-php/symfony-finder-2.7.20::GuillaumeSeren
>>> Emerging (8 of 19) dev-php/jsonlint-1.4.0::GuillaumeSeren
>>> Installing (8 of 19) dev-php/jsonlint-1.4.0::GuillaumeSeren
>>> Emerging (9 of 19) dev-php/symfony-filesystem-2.7.20::GuillaumeSeren
>>> Installing (9 of 19) dev-php/symfony-filesystem-2.7.20::GuillaumeSeren
>>> Emerging (10 of 19) dev-php/json-schema-2.0.0::GuillaumeSeren
>>> Installing (10 of 19) dev-php/json-schema-2.0.0::GuillaumeSeren
>>> Emerging (11 of 19) dev-php/phar-utils-1.0.0::GuillaumeSeren
>>> Installing (11 of 19) dev-php/phar-utils-1.0.0::GuillaumeSeren
>>> Emerging (12 of 19) dev-php/ca-bundle-1.0.0::GuillaumeSeren
>>> Installing (12 of 19) dev-php/ca-bundle-1.0.0::GuillaumeSeren
>>> Emerging (13 of 19) dev-php/cli-prompt-1.0.0::GuillaumeSeren
>>> Installing (13 of 19) dev-php/cli-prompt-1.0.0::GuillaumeSeren
>>> Emerging (14 of 19) dev-php/symfony-yaml-2.1.0::GuillaumeSeren
>>> Installing (14 of 19) dev-php/symfony-yaml-2.1.0::GuillaumeSeren
>>> Emerging (15 of 19) dev-php/symfony-config-2.1.0::GuillaumeSeren
>>> Installing (15 of 19) dev-php/symfony-config-2.1.0::GuillaumeSeren
>>> Emerging (16 of 19) dev-php/symfony-dependency-injection-2.1.0::GuillaumeSeren
>>> Installing (16 of 19) dev-php/symfony-dependency-injection-2.1.0::GuillaumeSeren
>>> Emerging (17 of 19) dev-php/symfony-event-dispatcher-2.1.0::GuillaumeSeren
>>> Installing (17 of 19) dev-php/symfony-event-dispatcher-2.1.0::GuillaumeSeren
>>> Emerging (18 of 19) dev-php/symfony-console-2.7.9::GuillaumeSeren
>>> Installing (18 of 19) dev-php/symfony-console-2.7.9::GuillaumeSeren
>>> Emerging (19 of 19) dev-php/composer-1.2.2::GuillaumeSeren
>>> Installing (19 of 19) dev-php/composer-1.2.2::GuillaumeSeren
>>> Recording dev-php/composer in "world" favorites file...
>>> Jobs: 19 of 19 complete                         Load avg: 4.12, 4.17, 4.36

 * Messages for package dev-php/phpunit-4.3.1:

 * phpunit can optionally use json, pdo-sqlite and pdo-mysql features.
 * If you want those, emerge dev-lang/php with USE="json pdo sqlite mysql".
>>> Auto-cleaning packages...

GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 27, 2016
GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 28, 2016
GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 28, 2016
Needed for the mgorny feedback,
```
As a nice trick, you could use . instead of * to avoid in-place filename expansion. This makes it a bit faster, a bit cleaner and ensures that dotfiles are installed as well.
```

gentoo/gentoo#2905 (comment)
GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 28, 2016
GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Nov 28, 2016
@GuillaumeSeren GuillaumeSeren force-pushed the dev-php/composer-1.2.2-add-ebuild branch from 2832c9f to e6e3f08 Compare November 30, 2016 15:00
@GuillaumeSeren
Copy link
Contributor Author

Hi there,
I have just finish rebase all the commits,
@mgorny, @orlitzky, if you want to have a look !

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

Almost there. Left a few minor comments. They apply to multiple packages, I've only left them on one of them.


RDEPEND="dev-lang/php:*"
DEPEND="test? ( ${RDEPEND}
dev-php/phpunit )"
Copy link
Member

Choose a reason for hiding this comment

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

Looks like indent gone wrong ;-).

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Dec 1, 2016

Choose a reason for hiding this comment

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

Yes I will them both RDEPEND and phpunit to a new line, will be much clean, like that:

DEPEND="
	test? (
		${RDEPEND}
		dev-php/phpunit
	)"

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that looks nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

RDEPEND="dev-lang/php:*
=dev-php/fedora-autoloader-0.2.1"
DEPEND="test? ( ${RDEPEND}
dev-php/phpunit )"
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Dec 1, 2016

Choose a reason for hiding this comment

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

In the case of the RDEPEND do you prefer everything on a new line, so do not try to fit something after 'RDEPEND="' ?

RDEPEND="
	dev-lang/php:*
	=dev-php/fedora-autoloader-0.2.1"

Copy link
Member

Choose a reason for hiding this comment

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

Entirely your choice. The only thing I'm complaining about is using more than one tab for indent (nesting) level. The idea is to do things like:

FOO="
<1-tab>    bar                 # variable scope
<1-tab>    baz? (
<2-tab>        bazbar          # 1 nested block scope
<2-tab>        bazbaz? (
<3-tab>             foobaz     # 2 nested blocks scope
...

It's fine to close braces either on the same line as last text, or in line following it with indent matching opening brace keyword. However, the latter is recommended when the conditions become complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I understand, I'll close the brace on the same line as last text, to remain consistent and short, and close the brace indented with the opening on a new line for the unusual/complex cases.

src_prepare() {
default
if use test; then
cp "${FILESDIR}"/autoload.php "${S}"/autoload-test.php
Copy link
Member

Choose a reason for hiding this comment

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

All external programs need ||die (cp, sed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i'll add it in the next upgrade

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Dec 1, 2016

Choose a reason for hiding this comment

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

@mgorny Does the die need a comment in sed / cp or just comments for tests ?

Copy link
Member

Choose a reason for hiding this comment

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

If by comment you mean a message, then no. We only use messages when the line itself does not provide good context. Here the failure is highly unlikely, and the line itself tells what failed.

Copy link
Contributor Author

@GuillaumeSeren GuillaumeSeren Dec 2, 2016

Choose a reason for hiding this comment

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

Yes by comment I mean messages, but scouting into some others ebuild of the tree show me that it is used in sed/cp case.

GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Dec 2, 2016
GuillaumeSeren added a commit to GuillaumeSeren/gentoo-overlay that referenced this pull request Dec 2, 2016
@GuillaumeSeren GuillaumeSeren force-pushed the dev-php/composer-1.2.2-add-ebuild branch from e6e3f08 to f96b000 Compare December 2, 2016 15:03
@GuillaumeSeren
Copy link
Contributor Author

Hey @mgorny ,
I have just finish fixing all your feedback so here is my upgrade.

@mgorny
Copy link
Member

mgorny commented Dec 2, 2016

LGTM though I'd prefer if someone who knows more about PHP than to hate it could take a look ;-). @gentoo/php, I mean ;-).

@GuillaumeSeren
Copy link
Contributor Author

GuillaumeSeren commented Dec 2, 2016

LGTM

Great ! 🍺

@orlitzky
Copy link
Contributor

Ok, I'm working on this again.

Unfortunately, I think the fig-log package has the wrong name. We're installing it into the Fig\Log namespace, but upstream and Fedora use Psr\Log instead. See for example,

https://github.com/php-fig/log/blob/master/composer.json
http://pkgs.fedoraproject.org/cgit/rpms/php-PsrLog.git/

@orlitzky
Copy link
Contributor

I think the names of the symfony packages are fine, but we're installing into e.g. Symfony/Process where upstream uses Symfony/Component/Process. That doesn't matter as far as composer is concerned, but if a user simply wants to use dev-php/symfony-process (and not Composer), then he'll be looking at these docs:

https://symfony.com/doc/current/components/process.html

The same goes for all of the symfony-* packages. Thinking towards the future, if we ever want to have one big "symfony" package, we'll need everything to be in the expected upstream location.

@orlitzky
Copy link
Contributor

Here's a first draft of the packaging guidelines based on your work:

https://wiki.gentoo.org/wiki/Project:PHP/Composer_packaging

Once we get this PR merged, I'll figure out how to get you write access to that part of the wiki.

@orlitzky
Copy link
Contributor

I have a few more minor comments, but I think we're pretty much ready to merge this.

First, json-schema and jsonlint still depend on a specific version of fedora-autoloader. You can almost certainly drop that to simply "dev-php/fedora-autoloader", since those packages don't even know that the fedora autoloader exists.

Second, the json-schema package is installing into the JsonSchema2 namespace. I see that Fedora is doing the same thing, but is there any other reason for us to do it? I only ask because the upstream docs all use JsonSchema (without the 2).

The "sed" line in the symfony-yaml ebuild (and maybe the others?) seems redundant -- the tests pass without it.

And finally, the $vendorDir variable appears in a few autoload.php files where it's never used (like psr-log and symfony-yaml). They're not hurting anything though.

I'm curious about the JsonSchema2 thing, but the rest aren't deal-breakers. There's one more problem that I hit with the test suites, but it's not your fault -- we added php-7.1 to the tree in the meantime and I don't think some of these packages are compatible. I'm not sure what to do about that, probably just restrict the test suites for now. I'll think about it tonight.

@GuillaumeSeren
Copy link
Contributor Author

GuillaumeSeren commented Dec 17, 2016

Hey @orlitzky

About the package naming

Slashes can be changed to hyphens, since slashes aren't allowed in our package names. If that name causes a conflict or is otherwise bad for some reason, we'll have to pick another one, but you'll have a hard time coming up with an exact rule.

I agree, I just try to keep the package name accessible, some name are aliased in packagist, some project name are not quite informative (too short or too different from the packagist key).
Let's change as little as we can, try to keep the packagist key visible (searchable), completing when needed by project name namespace.

About the version matching

If something depends on >=dev-php/lib-1.2.3 and every version of dev-php/lib in the tree is newer than that, then the >= can be dropped without hurting anything. It can also be left in without hurting anything. It's your choice =)

Again, it is about keeping the information, so if we can, I think using such '>=dev-php/lib-1.2.3',
is better because it didn't fix to a specific version, but keep the information given in the 'composer.json' (fedora do the same).
The case of older version of libs required, can / could append in that case we will have to reflect it, but has I talked to Remi from fedora, he told me to try to pick always last release, so that didn't work we will change, but I didn't get the case yet.

About the wiki page

Once we get this PR merged, I'll figure out how to get you write access to that part of the wiki.

Yes I could help with that, and maybe the page can motivate some other people to package / maintain the stack.

First, json-schema and jsonlint still depend on a specific version of fedora-autoloader. You can almost certainly drop that to simply "dev-php/fedora-autoloader", since those packages don't even know that the fedora autoloader exists.

Yes I will fix it.

Second, the json-schema package is installing into the JsonSchema2 namespace. I see that Fedora is doing the same thing, but is there any other reason for us to do it? I only ask because the upstream docs all use JsonSchema (without the 2).

Not really any opinion here, I followed the fedora choice, but as I remember the lib is a fork, and so there's 2 with the same name, I think that's why.

The "sed" line in the symfony-yaml ebuild (and maybe the others?) seems redundant -- the tests pass without it.

Yes in some case it will work, but it is required to be able to run test without having the lib already installed,, in all case I think it is good to have it to be sure we don't rely on installed lib to run the tests.

EDIT: After thinking about it, it should not be required because it is based on 'DIR',
but when I dev it was needed (to run tests obviously), but I will check if we can delete it without breaking the test, if it work, I'll delete them.

And finally, the $vendorDir variable appears in a few autoload.php files where it's never used (like psr-log and symfony-yaml). They're not hurting anything though.

Yes they are remaining of the dev and I will delete it.

There's one more problem that I hit with the test suites, but it's not your fault -- we added php-7.1 to the tree in the meantime and I don't think some of these packages are compatible.

I didn't test with php7 but I will check with that for the next upgrade (1.3) and fabpot from symfony just announce that he will probably drop php5 on the next version (https://twitter.com/fabpot/status/809743412715016192).

@orlitzky
Copy link
Contributor

I think dropping the "2" from JsonSchema is a good choice, but I'll leave it up to you. Changing it in the future would be annoying because all of the autoload.php files would need to be updated at the same time, probably with a symlink for backwards-compatibility. (Naming is the only thing you really want to get right the first time around, because it's the hardest to change.)

About the tests and php-7.x support... the "sed" line is needed for jsonlint, but not any of the symfony packages.

  • php-5.6 and phpunit-4.3.1: all of the tests pass.
  • php-7.1 and phpunit-4.3.1: fedora-autoloader, symfony-config, and symfony-event-dispatcher pass; all others fail.
  • php-5.6 and phpunit-5.0.10: fedora-autoloader, jsonlint, symfony-config, and symfony-event-dispatcher pass; the others fail.

Based on those results, I think we should leave the tests enabled for,

  • fedora-autoloader
  • symfony-config
  • symfony-event-dispatcher

but add RESTRICT=test to the others. Once we get the PR merged, it will be easier to play around with the others to see if we can make them work.

@GuillaumeSeren
Copy link
Contributor Author

@orlitzky I have worked a bit on the ebuilds.

I think dropping the "2" from JsonSchema is a good choice, but I'll leave it up to you.

Ok I'll change it !

About the tests and php-7.x support... the "sed" line is needed for jsonlint, but not any of the symfony packages.

Yes I confirm that.

php-5.6 and phpunit-5.0.10: fedora-autoloader, jsonlint, symfony-config, and symfony-event-dispatcher pass; the others fail.

Yes it is mainly due to big changes in phpunit, but there also to take in consideration some of the libs are a bit old and the test suite may have been fixed since, so we need to put everything up-to-date phpunit and libs to be sure that some lib break or not with different php version.

I try to activate tests when they work, but I didn't expect the result to change that much by upgrading phpunit or PHP itself, so as you suggest to restrict all of them (except 3), I will re add all test part, so we still can run them if we want.

This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This is needed for composer-1.2.2, bug 439206.
This package is a source alternative to the phar one.
It use dev-php/fedora-autoloader to provide a PSR4 loader,
for itself (composer) and all the dependencies, bug 439206.

Suggested-by: Michael Orlitzky <mjo@gentoo.org>
@GuillaumeSeren GuillaumeSeren force-pushed the dev-php/composer-1.2.2-add-ebuild branch from 3d91ead to 391cee7 Compare December 18, 2016 17:10
@GuillaumeSeren
Copy link
Contributor Author

GuillaumeSeren commented Dec 18, 2016

Hey @orlitzky
I have finished rebase all the patches, so

  • I add RESTRICT="test" on all ebuild (having tests) except the 3 you validated.
  • I remove the sed everywhere except on jsonlint.

@gentoo-repo-qa-bot
Copy link
Collaborator

👍 All QA issues have been fixed!

@orlitzky
Copy link
Contributor

and... it's merged! Thanks again!

@orlitzky orlitzky closed this Dec 18, 2016
@GuillaumeSeren
Copy link
Contributor Author

@orlitzky Oh great, thank you for help me out ! 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package.
Projects
None yet
4 participants