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

Fix: Filter undefined entries from loadExtensions array #3210

Closed
wants to merge 2 commits into from

Conversation

OISumeko
Copy link

This PR fixes an issue introduced in #3199, where the server would crash on startup if SQLEAN_UNICODE_PATH was not set.

As I'm not sure if loadExtensions will be used in other locations in the future, I figured filtering out undefined entries within the function might be preferable to to simply checking for a valid SQLEAN_UNICODE_PATH value before calling it.

@mikiher
Copy link
Contributor

mikiher commented Jul 30, 2024

Thanks for finding this!

That is not the proper fix, though. This extension is now required and BinaryManager should find it, fetch it if it's not found, or exit, and so the value of SQLEAN_UNICODE_PATH should always be set at this point.

I'm guessing that you have the env variable SKIP_BINARIES_CHECK=1. Instead of your fix, can you please remove the following lines from BinaraManager.js:

    // Optional skip binaries check
    if (process.env.SKIP_BINARIES_CHECK === '1') {
      Logger.info('[BinaryManager] Skipping check for binaries')
      return
    }

as the the Binary check is now not optional anymore.

Also, if you could also remove other references to SKIP_BINARIES_CHECK from the code.

@advplyr
Copy link
Owner

advplyr commented Jul 31, 2024

I think we should still allow passing in SKIP_BINARIES_CHECK but I agree that it should not be the default for the devcontainer dev.js.

I'm not sure that the binaries we are downloading will cover every OS and some users may need to supply their own and bypass the check. That was brought to my attention in #2741 (comment)

@mikiher
Copy link
Contributor

mikiher commented Jul 31, 2024

OK, but if we keep SKIP_BINARIES_CHECK, we also need to fail graecefully if any of the required environment variable are not set, like this:

    // Optional skip binaries check
    if (process.env.SKIP_BINARIES_CHECK === '1') {
      for (const binary of this.requiredBinaries) {
        if (!process.env[binary.envVariable]) {
          Logger.fatal(`[BinaryManager] Environment variable ${binary.envVariable} must be set`)
          process.exit(1)
        }
      }          
      Logger.info('[BinaryManager] Skipping check for binaries')
      return
    }

The code requires the unicode extension for any search to work now.

@advplyr
Copy link
Owner

advplyr commented Aug 1, 2024

I pushed this up here 4a5345d
Thanks!

@advplyr advplyr closed this Aug 1, 2024
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.

3 participants