Skip to content

Conversation

remicollet
Copy link
Contributor

Rebase against current master of pr #104

config.m4 Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

As to not make this confusing, perhaps we should use HAVE_SYSTEM_LIBBSON and HAVE_SYSTEM_LIBMONGOC? Because, we're always going to have a LIBBSON and LIBMONGOC.

@remicollet
Copy link
Contributor Author

@derickr macros renamed

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep this consistent.
Otherwise we have to ask what does phpinfo() report as "libmongoc headers version and "libmongoc library version" -- or -- libmongoc version?.

Lets just always print out both compile time and lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or "libbson bundled version" ?
(I'd like to make explicit if system or bundled library is used, which as some value tyring to help/support our users)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. Makes sense. Shall we make that the third line? mongoc: system/bundled ?
Then its easy ctrl+fable, along with being straight forward to ask/answer the question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think having 1 line for bundled library and 2 for system library is quite common and consistent with other ext : see GD, msgpack, libsodium ...)... ok.... all added by me ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice: I CANNOT change now, as bson_get_version is not yet defined in bundled lib... (need to wait for PR #119)

Copy link
Contributor

Choose a reason for hiding this comment

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

hehe. ok. This is such a minor detail that as long as the output isn't vastly different then it isn't worth spending to much time on this.

One thing that just hit me though, maybe we should update the trace line that reports these versions:
https://github.com/mongodb-labs/mongo-php-driver-prototype/blob/master/php_phongo.c#L1873

I am hoping the only question we will ever need to ask users for support cases is:

Can you change your php.ini to include mongodb.debug=/tmp/ and rerun the script as send us the content?

Or

Can you pass array("debug" => "/tmp/") as 3rd argument to the ctor

So it would be nice to know there if it was bundled/system.
See also: https://jira.mongodb.org/browse/PHPC-442

@remicollet
Copy link
Contributor Author

 $ php -n -d extension=$PWD/modules/mongodb.so -r '$manager = new MongoDB\Driver\Manager("mongodb://localhost:27017",[],["debug" => "/tmp/"]);'
 $ $ cat /tmp/PHONGO*
 [2015-10-05T17:56:56+00:00]     PHONGO: DEBUG   > Creating Manager, phongo-1.0.0beta2[devel] - mongoc-1.2.0-beta1(bundled), libbson-1.2.0-beta(bundled), php-5.6.14
 ....

@bjori
Copy link
Contributor

bjori commented Oct 5, 2015

Thanks!

jmikola added a commit that referenced this pull request Oct 5, 2015
@jmikola
Copy link
Member

jmikola commented Oct 5, 2015

Manually merged in 71b3cb3. Thanks, @remicollet!

@jmikola jmikola closed this Oct 5, 2015
Copy link
Member

Choose a reason for hiding this comment

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

This appears to trigger Compiler Error C2121 in VC11, because we're using a macro within the MONGOC_DEBUG() macro. I believe a quick solution would be to populate const char * variables beforehand and use those as-is in the MONGOC_DEBUG() macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this... :(

Copy link
Member

Choose a reason for hiding this comment

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

No worries! I've missed more than my fair share of Windows errors :)

5917787 should fix it.

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.

4 participants