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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trying to fix some of the HHVM unittests #12336

Merged
merged 2 commits into from Oct 29, 2016

Conversation

Projects
None yet
8 participants
@Hackwar
Member

Hackwar commented Oct 7, 2016

According to the documentation, a DSN connection string should use semicolons as delimiter and not spaces. It should also not make any difference for the normal PHP implementation. Since we have currently ~80 errors in the test suite when running on HHVM and most of these are related to this DSN string, I'm trying to fix this. Funnily, I don't have access to HHVM, so I'm going to try to fix this by running this through the unittests again and again... 馃檴 馃槃

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 7, 2016

Seems like this indeed "fixes" that error, however throwing a fatal error because of this: facebook/hhvm#7320

Seems as if we would only support the very latest version of HHVM with Postgres. Can we have such a requirement or do we have to introduce a conditional for that function call?

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 7, 2016

So... with this PR, the errors would be reduced from 80 to 11. Is it worth that?

@zero-24 zero-24 added this to the Joomla 4.0 milestone Oct 7, 2016

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 7, 2016

Yes it's worth

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Oct 7, 2016

can't this go for staging and 3.7.x too?

@mbabker mbabker changed the base branch from 4.0-dev to staging Oct 7, 2016

@mbabker mbabker changed the base branch from staging to 4.0-dev Oct 7, 2016

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 7, 2016

Well, I tried changing the base branch but the PR would've drug in the entirety of the 4.0 branch. But yes, this should go into staging instead of 4.0 only.

@zero-24

This comment has been minimized.

Contributor

zero-24 commented Oct 7, 2016

We can merge it against 4.0-dev and we can open a new PR against staging 馃槃

Done: #12341

@Hackwar Hackwar force-pushed the Hackwar:hhvmtests branch from 94c7a60 to b46ddc5 Oct 7, 2016

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 7, 2016

These are exactly the changes I have made in my test branches pending release of hhvm 3.15.2 or 3.16 which will resolve these issues. facebook/hhvm#7399 (comment)

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 7, 2016

BTW some of these issues were cataloged a while back in #10220 although the connection issue was new in 3.15 with the inclusion of pgsl.so in HHVM as noted in facebook/hhvm#7399 (comment)

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 7, 2016

@Hackwar According to the documentation spaces or semi-colons should work,

The PDO_PGSQL Data Source Name (DSN) is composed of the following elements, delimited by spaces or semicolons:

Fortunately HHVM is working on a fix. But I do agree this is a valid fix/work around (personally, I think in most cases spaces are the worst choice for delimiters)

I have been hesitant to submit the change for our call to pg_set_error_verbosity() since I'm unsure of any possible downstream consequences (I assume possibly none, but you know what they say about assuming). I'm also hesitant since this issue is fixed in hhvm 3.15.2. At the same time, at one point I suggested the same change as a possible work around.

looks good on code review.

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 11, 2016

So with those comments, we could merge this. 馃槃

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 16, 2016

The change to our call of pg_set_error_verbosity() is no longer needed as hhvm 3.15.2 has been released. We do still need the change to the separator for the DSN used in the tests as that patch has not been merged (they requested some additional changes).

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 17, 2016

The HHVM patch to fix the DSN separators to allow semicolon or space has been merged, the HHVM 3.16.0 STS release will have this, and is on the list of potential cherry picks for HHVM 3.15.3 LTS.

HHVM 3.16.0 STS is expected around 10/24/2016 and will automatically be the version tested on Travis CI since we are testing against HHVM latest.

@andrepereiradasilva

This comment has been minimized.

Contributor

andrepereiradasilva commented Oct 17, 2016

so all we have to do is wait?

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 17, 2016

@andrepereiradasilva There is no harm in merging as it's a semantics change to our unit test, but yes it will also be fixed by just waiting.

I would suggest backing out the pg_set_error_verbosity() change if we do chose to merge, as that portion of the PR is no longer needed as of HHVM 3.15.2.

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 17, 2016

Depends what we consider a minimum supported HHVM version to be. If it's something older than 3.15.2 it'll be needed.

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 17, 2016

@mbabker Since hhvm 3.15.0 is the first version with pgsql.so in core I would suggest that would be the earlist LTS version to consider. 3.15.2 is also the first version were tests do not error out on memory.

As noted in #10220 (comment) we can pick up any recent LTS version of HHVM for testing on Travis CI or latest or nightly. we can also test with and without HHVM's php 7 mode (although the HHVM php 7 mode is currently broken due to facebook/hhvm#7198 )

We still need a fix for facebook/hhvm#2060 and to solve the JInstallerAdapterTest failures or determine if the JInstallerAdapterTest failures are an HHVM bug before we can determine a minimum HHVM version to support. I've been pushing HHVM to include the important bug fixes we need in the patches of 3.15 so that hopefully 3.15 can be a valid LTS release to target for a supported version.

@wilsonge

This comment has been minimized.

Contributor

wilsonge commented Oct 18, 2016

Does the CMS actually work on HHVM practically (and by work I mean is the core extensions + functionality useable not just do the unit tests pass)

@mbabker

This comment has been minimized.

Member

mbabker commented Oct 18, 2016

Who actually runs HHVM outside of Travis to test that? 馃槅

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Oct 18, 2016

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 18, 2016

if anyone has a full siteground account, I believe that host does HHVM.

I've seen a few comments from people who tried running J3.4 on HHVM but ran into a lot of issues. Some of the past items have been fixed in the CMS or in HHVM.

At the moment the failures in the unit tests suggest to me that it's unlikely for the CMS to actually run on HHVM without issues. But I haven't tried running anything other than unit tests.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Oct 18, 2016

Only on their Cloud Hosting plans - not the regular ones

On 18 October 2016 at 15:12, Walt Sorensen notifications@github.com wrote:

if anyone has a full siteground account, I believe that host does HHVM.

I've seen a few comments from people who tried running J3.4 on HHVM but
ran into a lot of issues. Some of the past items have been fixed in the CMS
or in HHVM.

At the moment the failures in the unit tests suggest to me that it's
unlikely for the CMS to actually run on HHVM without issues. But I haven't
tried running anything other than unit tests.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12336 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8T1pOOAU5aWOpl3IIw--VI8q6tVFks5q1NO3gaJpZM4KQx4U
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Oct 18, 2016

and it is only an option not a default

On 18 October 2016 at 15:32, Brian Teeman brian@teeman.net wrote:

Only on their Cloud Hosting plans - not the regular ones

On 18 October 2016 at 15:12, Walt Sorensen notifications@github.com
wrote:

if anyone has a full siteground account, I believe that host does HHVM.

I've seen a few comments from people who tried running J3.4 on HHVM but
ran into a lot of issues. Some of the past items have been fixed in the CMS
or in HHVM.

At the moment the failures in the unit tests suggest to me that it's
unlikely for the CMS to actually run on HHVM without issues. But I haven't
tried running anything other than unit tests.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12336 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8T1pOOAU5aWOpl3IIw--VI8q6tVFks5q1NO3gaJpZM4KQx4U
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 18, 2016

Best priced options for HHVM that I could find on a quick search was Google cloud at $5/mo or free trial and A2 Hosting at $5/mo

red hat openshift claims to have a free HHVM option in their developer preview, but I couldn't get it to launch into my account.

Heroku says it offers HHVM as a "highly experimental option", but I haven't had time to deal with a platform that relies heavily on command line for setup and activation. I was looking at heroku as an option since heroku has a free option that can be sandboxed for testing.

@brianteeman

This comment has been minimized.

Contributor

brianteeman commented Oct 18, 2016

The question is "are there any users with hhvm"

On 18 October 2016 at 16:45, Walt Sorensen notifications@github.com wrote:

Best priced options for HHVM that I could find on a quick search was Google
cloud at $5/mo or free trial
https://console.cloud.google.com/launcher/details/bitnami-launchpad/hhvm
and A2 Hosting at $5/mo https://www.a2hosting.com/hhvm-hosting

red hat openshift claims to have a free HHVM option
https://hub.openshift.com/quickstarts/127-hhvm in their developer
preview, but I couldn't get it to launch into my account.

Heroku says it offers HHVM as a "highly experimental option"
https://devcenter.heroku.com/articles/php-support#selecting-a-runtime-hhvm,
but I haven't had time to deal with a platform that relies heavily on
command line for setup and activation. I was looking at heroku as an option
since heroku has a free option that can be sandboxed for testing
https://www.heroku.com/pricing.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#12336 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8XSbWbwK-xz0BnspQ4F-Tl_dMz_Qks5q1OmzgaJpZM4KQx4U
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
https://brian.teeman.net/ http://brian.teeman.net/

@photodude

This comment has been minimized.

Contributor

photodude commented Oct 18, 2016

The question is "are there any users with hhvm"

As I said, I've seen a few comments from people who tried running J3.4.6 on HHVM but ran into a lot of issues. I haven't seen anything more recent.

@wilsonge wilsonge merged commit b3a112b into joomla:4.0-dev Oct 29, 2016

2 checks passed

continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@photodude

This comment has been minimized.

Contributor

photodude commented Oct 30, 2016

We still need the DSN separator portion of this backported to staging since #12341 was closed. (of course, it won't be needed whenever HHVM 3.16.0 gets released in the next few weeks or hopefully 3.15.3 if they merge the fix for the release in the next few weeks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment