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

Trying adding alternative dependecies for the phpparser #650

Merged
merged 7 commits into from
Jun 3, 2017

Conversation

Skullbock
Copy link

No description provided.

@photodude
Copy link

You need to regenerate the composer.lock file on your local and include that new file.

composer.json Outdated
@@ -4,7 +4,7 @@
"description": "FOF (Framework on Framework) is a Rapid Application Development framework for Joomla!",
"require" : {
"php": ">=5.3.4",
Copy link

@photodude photodude Feb 2, 2017

Choose a reason for hiding this comment

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

Try this and redo the lock file as nothing with nikic/php-parser changed in the last update

   "require": {
        "php": "^5.3.10|^5.6",
        "nikic/php-parser": "~2.1|~3.0"
    },

@Skullbock
Copy link
Author

i updated the entire composer.lock (composer update). I hope that's all right, otherwise i'll just do composer update nikic/php-parser

@photodude
Copy link

Something is not quite right on php 5.3

Problem 1
    - Installation request for nikic/php-parser v3.0.2 -> satisfiable by nikic/php-parser[v3.0.2].
    - nikic/php-parser v3.0.2 requires php >=5.5 -> your PHP version (5.4.37) does not satisfy that requirement.
  Problem 2
    - Installation request for phpdocumentor/reflection-common 1.0 -> satisfiable by phpdocumentor/reflection-common[1.0].
    - phpdocumentor/reflection-common 1.0 requires php >=5.5 -> your PHP version (5.4.37) does not satisfy that requirement.
  Problem 3
    - Installation request for phpdocumentor/reflection-docblock 3.1.1 -> satisfiable by phpdocumentor/reflection-docblock[3.1.1].
    - phpdocumentor/reflection-docblock 3.1.1 requires php >=5.5 -> your PHP version (5.4.37) does not satisfy that requirement.
  Problem 4
    - Installation request for phpdocumentor/type-resolver 0.2.1 -> satisfiable by phpdocumentor/type-resolver[0.2.1].
    - phpdocumentor/type-resolver 0.2.1 requires php >=5.5 -> your PHP version (5.4.37) does not satisfy that requirement.
  Problem 5
    - phpdocumentor/reflection-docblock 3.1.1 requires php >=5.5 -> your PHP version (5.4.37) does not satisfy that requirement.
    - phpspec/prophecy v1.6.2 requires phpdocumentor/reflection-docblock ^2.0|^3.0.2 -> satisfiable by phpdocumentor/reflection-docblock[3.1.1].
    - Installation request for phpspec/prophecy v1.6.2 -> satisfiable by phpspec/prophecy[v1.6.2].

@Skullbock
Copy link
Author

The first one is the one that bothers me. Why doesn't it take the 2.0 version? mmmm

@photodude
Copy link

It's something with the composer.lock file... looking into it

@photodude
Copy link

photodude commented Feb 2, 2017

I'm not sure but Try this with double pipes and an expanded php range

{
	"name": "akeeba/fof",
	"type": "library",
	"description": "FOF (Framework on Framework) is a Rapid Application Development framework for Joomla!",
	"require": {
		"php": "~5.3.10||>=5.4,<5.5||>=5.5",
		"nikic/php-parser": "~1.0||~2.0||~3.0"
	},
	"keywords": ["framework","joomla","mvc","rad"],
	"homepage": "https://github.com/akeeba/fof",
	"license": "GPL-2.0+",
	"authors": [
		{
			"name": "Nicholas K. Dionysopoulos",
			"email": "nicholas_NO_SPAM_PLEASE@akeebabackup.com",
			"homepage": "http://www.dionysopoulos.me",
			"role": "Lead Developer"
		}
	],
	"require-dev": {
		"mikey179/vfsStream": "dev-master",
		"phpunit/phpunit": "~4.8||~5.7",
		"phpunit/dbunit": "~1.4||~2.0",
		"mockery/mockery": "0.9.4||~0.9.5",
		"fiunchinho/phpunit-randomizer": "1.0.*@dev"
	},
	"autoload" : {
		"psr-4": {
			"FOF30\\": "fof"
		}
	}
}

@photodude
Copy link

The json above works (it's a bit ugly, but it works)

I'm not sure if it will work with the lock file, so I'm looking at alternatives

@Skullbock
Copy link
Author

Skullbock commented Feb 2, 2017 via email

@photodude
Copy link

Seems something is wrong with the other parts of the lock file now (from the previous lock file update at 6bf6c85 )

@photodude
Copy link

I did a test without the lock file https://travis-ci.org/photodude/fof/builds/197738151 and everything works, so just need to sort out the lock file issues.

Maybe try deleting the lock file and create a new one.

@Skullbock
Copy link
Author

Tried it, let's see :)

@Skullbock
Copy link
Author

This is sooo strange...

@photodude
Copy link

I'm guessing the composer lock file is somehow dependent on the php version it is created on, and you created it on php 5.6 or php 7. I have no idea if there is a way to generate a generic lock file that works for the different version ranges.

@Skullbock
Copy link
Author

I could generate it on php56 so travis is happy, let me try

@Skullbock
Copy link
Author

This is generated on 5.4 and seems to read the correct version...

@Skullbock
Copy link
Author

fails anyway, should we put it into require-dev and let it go?

@photodude
Copy link

Looks like there may be only two solutions, (which I think is bad on either side)

  • add --ignore-platform-reqs to the travis.yml composer install line (bad because it ignores all platform requirements for all packages)
  • remove the lock file from the repo (bad for all the reasons we should have a lock file in the repo)
    • maybe we could figure out how to delete the lock file before the composer install on travis so we can bypass the lock file issues (seems like a very hackish way of getting around the issue)

Considering we see this as a dev-only tool, it probably belongs in the require-dev section anyway (although I think require-dev means required for developing the package not required for developers)

@Skullbock
Copy link
Author

Yeah it would mean that, but who'll use this tool will probably be an advanced user so...

@photodude
Copy link

Moving it to require-dev solves portability for consuming packages, but does not solve the travis CI testing issues as that runs in the require-dev mode.

@Skullbock
Copy link
Author

I really don't know what to say then...
Not that much of a travis expert unfortunately

@photodude
Copy link

It's a composer issue, not travis (All I know about composer is what I have read and have seen others do).

@Skullbock
Copy link
Author

What if we composer update in the travis job? That way the composer.lock file would be regenerated by travis on the fly, respecting the dependencies?

@photodude
Copy link

maybe that could work, I would delete the lock file in travis before the composer update. Like I said, probably a bit of a hackish solution but sometimes we have no other option.

@Skullbock
Copy link
Author

@nikosdion @tampe125 What do you guys think?

@nikosdion
Copy link
Member

None of these approaches make me happy :/

Removing the lock file is bad because each developer working on this repository will end up working against different versions of Composer packages.

require-dev is only to be used for development. This is not the case, you need that code for the tool.

That said, adding the requirements into the mail composer.json is equally bad. Keep in mind that the CLI tool is to be packaged as a PHAR archive. If your vendor folder is outside the tool's structure, how is it going to be packaged in it?.

It seems to me that the best temporary solution is to create a new composer.json inside the tool's directory. This is an architecturally correct solution, too! The CLI tool is independent of the main repo and vice versa. So I'd say do this instead.

Finally, the permanent solution is to, of course, move the tool to a new repository but that's something I am not willing to do right now :)

@Skullbock
Copy link
Author

ok, i'll try to do that. How should i then load both composer.json in the cli tool?

@nikosdion
Copy link
Member

FOF is one thing. The CLI script is another thing. Two separate composer.json files for two separate things. One does not load the other.

@Skullbock
Copy link
Author

I meant that in the fof cli tool we use the fof generators, factories, etc.
Can i just put fof as a composer dependency and that's it?

@nikosdion
Copy link
Member

No. The CLI script needs to be configured with the site's root. Then it loads FOF from the site's libraries/fof30 directory. You don't need to add FOF as a Composer dependency. FOF is something you must install on the site before using this tool.

@Skullbock
Copy link
Author

Ok, so the only dependencies i need to specify are the tool's one, and those have to be removed from the fof library. Ok

@photodude photodude mentioned this pull request Jun 2, 2017
@nikosdion nikosdion merged commit 01a95df into akeeba:development Jun 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants