Skip to content

Conversation

@aasawariMongoDB
Copy link
Contributor

The PR contains fixes for:

  1. Updating with steps to install openssl before installing MongoDB.
  2. Updating the script with correct field values. Someone doing it themselves will runa script that has mismatched fieldnames in the collection that the Documents in the Symfony code.

workship with steps to install openssl and compile using openssl and also using correct field nam
es to the script files
Comment on lines 25 to 28
export OPENSSL_DIR=$(brew --prefix openssl)
export LDFLAGS="-L$OPENSSL_DIR/lib"
export CPPFLAGS="-I$OPENSSL_DIR/include"
export PKG_CONFIG_PATH="$OPENSSL_DIR/lib/pkgconfig"
Copy link

Choose a reason for hiding this comment

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

@jmikola: when running ./configure, I typically used --with-mongodb-ssl=openssl --with-openssl-dir=$(brew prefix openssl). According to the pecl documentation, these options could be passed to pecl as well:

pecl install mongodb -D "with-mongodb-ssl=openssl with-openssl-dir=$(brew prefix openssl)"

In the long term we should try to simplify this, but I wanted to double check if there might be any problems with the LDFLAGS approach above, or whether this is a good workaround for MacOS until auto-detection of OpenSSL installed through homebrew works.

Copy link

Choose a reason for hiding this comment

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

Why is this necessary if OpenSSL is already preferred since 1.17 (see: PHPC-2280)? I'll note that we also removed support for --with-openssl-dir in 2.0 (PHPC-2309).

When setting up my MacBook, I installed openssl@3 via Homebrew and had no issues with configure detecting it properly.

Copy link

Choose a reason for hiding this comment

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

This should not be necessary if pkg-config is available (pkgconf Homebrew package), as noted in our PECL install docs.

Copy link

Choose a reason for hiding this comment

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

Yep, that's what makes all the difference.

@aasawariMongoDB - can we document that people should install the pkgconf package from homebrew as a requirement instead of using LDFLAGS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Added to the new commit.

"night_cost": 415,
"total_cost": 1245,
"start_date": {
"rental": "65dca21600a610ba8536b343",
Copy link

Choose a reason for hiding this comment

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

This looks like it's an ObjectID - is this stored as a string on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an ObjectID. Its a DbRef to the Rentals collections.
I have added the fix to the new commit as
"rental": {"$ref":"rentals","$id":{"$oid":"65dca21600a610ba8536b343"}},

export LDFLAGS="-L$OPENSSL_DIR/lib"
export CPPFLAGS="-I$OPENSSL_DIR/include"
export PKG_CONFIG_PATH="$OPENSSL_DIR/lib/pkgconfig"
brew install pkgconf
Copy link

Choose a reason for hiding this comment

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

Can we trust that openssl@3 will already be installed in Homebrew? Just asking as that's what we're expecting to find with pkg-config.

Copy link

Choose a reason for hiding this comment

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

Homebrew includes an alias for openssl that points to openssl@3, so unless people explicitly install openssl@1 they'll get the expected version.

I am a bit curious about "expecting to find" here - we're looking for openssl >= 1.0.1 in CheckSSL.m4, so I'm not sure why we're expecting to find openssl 3.

Copy link

@jmikola jmikola Feb 26, 2025

Choose a reason for hiding this comment

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

Apologies. I just saw my local package and forgot that CheckSSL.m4 only requires 1.0.1+ when I wrote that.

It was less about suggesting version 3+ and more about whether we should tell users they need openssl itself installed (to avoid falling back to Secure Transport). I don't recall installing it manually in Homebrew. I think it was just pulled in by some other packages.

Copy link

Choose a reason for hiding this comment

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

Yes, we may consider suggesting people install openssl as well - even though there is a high likelihood that it will have been installed as a dependency for other development tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.


<Tabs>
<TabItem value="Mac">
With MacOS, users will need to make sure to install pkgconf from homebrew as mentioned [here](https://formulae.brew.sh/formula/pkgconf) using the command
Copy link

Choose a reason for hiding this comment

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

Suggested change
With MacOS, users will need to make sure to install pkgconf from homebrew as mentioned [here](https://formulae.brew.sh/formula/pkgconf) using the command
With MacOS, users will need to make sure to install pkg-config from Homebrew as mentioned [here](https://formulae.brew.sh/formula/pkgconf) using the command

I'm not sure how this will be rendered, but generally our docs prefer contextual names for links instead of terms like "here". So in that case it may be more helpful to say something like:

...make sure that pkg-config is installed using the pkgconf package in Homebrew

Small nit, but the software (and installed binary) is pkg-config even though Homebrew calls the package pkgconf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@aasawariMongoDB
Copy link
Contributor Author

Merging the PR.

@aasawariMongoDB aasawariMongoDB merged commit 26b907a into main Mar 13, 2025
1 check 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.

4 participants