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

modified build.xml to allow mysql root user without password #226

Merged
merged 1 commit into from
Nov 4, 2014

Conversation

smoebody
Copy link

Hello,

since we want to test what we develop we should provide the developer with the required environment.
This adds the composer.phar and the dev-dependencies like phing, phpunit, etc.

Also the changes in build.xml allow to use -Dmysqlrootpass='' if the root user has no password.

I would have preferred not to set the dependencies under version control, but you actually have to own a github accout to successful install all dependencies, so its no option to leave this job to jenkins.

This is more of a suggestion than a must have, although it would help to integrate unittests in our workflow. All comments are appreciated.

Greetings from Leipzig University Library,
Ulf Seltmann

@demiankatz
Copy link
Member

Any chance you could open a separate PR for the build.xml improvement? It sounds like that's an easy thing to merge, while the rest of this will require more thought. I'm beginning to lean more toward avoiding bundling dependencies as the size of the vendor directory grows and grows, and adding 10,000 more files to the default distribution is not particularly attractive!

Which dependencies require a GitHub account to install? I wonder if we can find a better way around that limitation....

@smoebody
Copy link
Author

smoebody commented Nov 3, 2014

yes i will make a separate pull request for the build.xml.

Regarding the composer issue:
when i want to install sebastian bergmanns phpunit testing suite it asks for the github user credentials. this limitation is known by the composer team and handled by creating an oauth-token in github which can be added to the composer.json (see https://getcomposer.org/doc/articles/troubleshooting.md)

in my opinion its a problem of consistency to have some composer-pulled resources under version control and others not. it should be all or nothing and thats why i choose to but the dev-dependencies under version control as well.
i would like to clean this up and remove the vendor folder from the sources but i would only do that, if its for your sake.

tell me what you think

Ulf

@smoebody smoebody changed the title added composer.phar and dev-dependencies from ci-tasks modified build.xml to allow mysql root user without password Nov 3, 2014
@demiankatz
Copy link
Member

Thanks for revising the PR. One question before I merge: is there a reason that you removed the default value of the property in favor of an if/else? I'm guessing that there may be some behavior in Phing where a default property value overrides a blank string, in which case this approach makes sense... but if that's not the case, it seems like it would be more efficient to simply have a single if check that looks for the blank string and either adds the -p prefix or leaves the blank string alone.

Regarding the composer issue, it's good to know that there's a way to work around it; thanks for the troubleshooting link. I think we'll need to have some more conversations among the community before making changes, and it won't be too hard to implement those changes if we come to a decision, so there's no need for you to go to any trouble making a PR for cleaning up vendor. However, if you'd like to open a PR with just your composer.json changes as a reminder and a place for having related conversations, please feel free.

@smoebody
Copy link
Author

smoebody commented Nov 4, 2014

You are totally right, i rebased my changeset to fix this.

i will present something to engage the composer-issue. maybe we continue our discussion there.

demiankatz added a commit that referenced this pull request Nov 4, 2014
modified build.xml to allow mysql root user without password
@demiankatz demiankatz merged commit 26b02f9 into vufind-org:master Nov 4, 2014
@demiankatz
Copy link
Member

Thanks!

@demiankatz
Copy link
Member

For some reason, merging this PR is breaking Phing for me.

[phingcall] Calling Buildfile ' with target 'startup'
vufind2 > startup:
      [php] Evaluating PHP expression: end(explode('/', 'http://vufind.org/vufind_ci'))
     [copy] Copying 1 file to /etc/httpd/conf.d
     [echo] Stopping httpd: [  OK  ]
Starting httpd: [  OK  ]
       [if] Error in IfTask
Execution of target "startup" failed for the following reason: :126:8: Error in IfTask
#0 /opt/rh/php54/root/usr/share/pear/phing/UnknownElement.php(96): TaskAdapter->main()
#1 /opt/rh/php54/root/usr/share/pear/phing/Task.php(260): UnknownElement->main()
#2 /opt/rh/php54/root/usr/share/pear/phing/Target.php(297): Task->perform()
#3 /opt/rh/php54/root/usr/share/pear/phing/Target.php(320): Target->main()
#4 /opt/rh/php54/root/usr/share/pear/phing/Project.php(824): Target->performTasks()
#5 /opt/rh/php54/root/usr/share/pear/phing/tasks/system/PhingTask.php(277): Project->executeTarget('startup')
#6 /opt/rh/php54/root/usr/share/pear/phing/tasks/system/PhingTask.php(150): PhingTask->processFile()
#7 /opt/rh/php54/root/usr/share/pear/phing/tasks/system/PhingCallTask.php(158): PhingTask->main()
#8 /opt/rh/php54/root/usr/share/pear/phing/UnknownElement.php(96): PhingCallTask->main()
#9 /opt/rh/php54/root/usr/share/pear/phing/Task.php(260): UnknownElement->main()
#10 /opt/rh/php54/root/usr/share/pear/phing/Target.php(297): Task->perform()
#11 /opt/rh/php54/root/usr/share/pear/phing/Target.php(320): Target->main()
#12 /opt/rh/php54/root/usr/share/pear/phing/Project.php(824): Target->performTasks()
#13 /opt/rh/php54/root/usr/share/pear/phing/Project.php(797): Project->executeTarget('main')
#14 /opt/rh/php54/root/usr/share/pear/phing/Phing.php(586): Project->executeTargets(Array)
#15 /opt/rh/php54/root/usr/share/pear/phing/Phing.php(170): Phing->runBuild()
#16 /opt/rh/php54/root/usr/share/pear/phing/Phing.php(278): Phing::start(Array, NULL)
#17 /opt/rh/php54/root/usr/share/pear/phing.php(43): Phing::fire(Array)
#18 {main}

I've reverted the commit for now. Not sure if this means we're running different versions of Phing, or if your fix only works when you have a blank password, and there's a problem with how it handles the more common case with a password being passed in.

Any ideas? Thanks!

@smoebody
Copy link
Author

smoebody commented Nov 5, 2014

It seems like its not working in phing to overwrite a property with its own value. Should have tested it better, sorry for that. I will give it another try later.

@demiankatz
Copy link
Member

One approach might be to use <php expression="..." returnProperty="..." /> to create a new property (maybe called mysqlrootpassswitch) that prepends the switch to a new value rather than modifying the existing one in place...

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.

2 participants