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

Updated phing/phing to 3.0 #22

Merged
merged 4 commits into from
Jul 8, 2024
Merged

Updated phing/phing to 3.0 #22

merged 4 commits into from
Jul 8, 2024

Conversation

sturkel89
Copy link
Contributor

I updated build.xml and composer.json to be compatible with phing/phing v3.0.

@sturkel89 sturkel89 self-assigned this Jun 18, 2024
@sturkel89 sturkel89 requested a review from demiankatz June 18, 2024 19:48
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Didn't have time for a full review but noticed one small thing...

build.xml Outdated
</target>

<!-- Set up dependencies -->
<target name="startup" description="set up dependencies">
<exec command="composer install" />
<exec executable="composer install">
Copy link
Member

Choose a reason for hiding this comment

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

I think "install" needs to be an arg line 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.

Good catch! I just made that change. :)

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@sturkel89, thanks for the fixes. I've noticed a couple of new things that need attention, though you couldn't possibly have known about them (and I only learned about the second earlier today). I think once these adjustments are made, we can make this live, and the other projects should generally be pretty easy to bring in sync with this one since they're all set up quite similarly.

build.xml Outdated
</target>

<!-- PHP_Depend code analysis -->
<target name="pdepend">
<exec command="${srcdir}/vendor/bin/pdepend --jdepend-xml=${builddir}/reports/jdepend.xml --jdepend-chart=${builddir}/reports/dependencies.svg --overview-pyramid=${builddir}/reports/pdepend-pyramid.svg ${srcdir}/src" />
<target name="pdepend">
Copy link
Member

Choose a reason for hiding this comment

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

This whole pdepend block is no longer used and should be removed. (Not related to your changes -- but we might as well clean it up while we're updating things).

Copy link
Member

Choose a reason for hiding this comment

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

(The pdepend tool was discontinued last year... I removed it from composer.json and our Jenkins setup, but I must have forgotten to delete it from build.xml as well).

build.xml Outdated
</then>
</if>
</target>

<!-- PHPUnit -->
<target name="phpunit" description="Run tests">
<exec dir="${srcdir}" command="${phpunit_command} -dzend.enable_gc=0 --log-junit ${builddir}/reports/phpunit.xml --coverage-clover ${builddir}/reports/coverage/clover.xml --coverage-html ${builddir}/reports/coverage/" passthru="true" checkreturn="true" />
<exec dir="${srcdir}" executable="${phpunit_command}" passthru="true" checkreturn="true">
Copy link
Member

Choose a reason for hiding this comment

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

Ere and I finished upgrading Phing to 3.0 in the main project today, and we discovered a problem: in some cases, the phpunit_command property gets set to an executable plus arguments, and that breaks the new system here. The workaround was to create a new "sh" property that specifies the Unix shell command, and then make that the executable while putting everything else into the argument line.

Here's where the new property is set up at the top of the file:

https://github.com/vufind-org/vufind/blob/dev/build.xml#L3

And here is how things are put together in the phpunit commands:

https://github.com/vufind-org/vufind/blob/dev/build.xml#L369-L370

So, for example, these two lines would become:

<exec dir="${srcdir}" executable="${sh}" passthru="true" checkreturn="true">
  <arg line="-c &apos;${phpunit_command} -dzend.enable_gc=0 --log-junit ${builddir}/reports/phpunit.xml --coverage-clover ${builddir}/reports/coverage/clover.xml --coverage-html ${builddir}/reports/coverage/&apos;" />

(Note the -c &apos;${phpunit_command} ... &apos; wrapped around everything to make it a command to run with the shell).

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @sturkel89!

@demiankatz demiankatz merged commit a51e814 into vufind-org:dev Jul 8, 2024
3 checks passed
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