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

sqlite .db file path may be incompatible between Windows and *nix when using Sequel #698

Closed
yasuyuki opened this Issue Oct 11, 2011 · 9 comments

Comments

Projects
None yet
4 participants
@yasuyuki

yasuyuki commented Oct 11, 2011

padorino generator creates database.rb like

Sequel.connect("sqlite://" + Padrino.root('db', "project_name_development.db"), :loggers => [logger])

on Windows, Padrino.root returns like "c/path/to/project_name", so Sequel connection url doesn't have enough slashes and doesn't work.
rewrite as

Sequel.connect("sqlite:///" + Padrino.root('db', "project_name_development.db"), :loggers => [logger])

then it works. but it may not work on *nix. (not tested yet.)

@ghost ghost assigned nesquena Oct 12, 2011

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Oct 26, 2011

Member

Ok we need to make sure this will work on both unix and windows. Is there a way we can rewrite this to be compatible with both? @DAddYE? any ideas

Member

nesquena commented Oct 26, 2011

Ok we need to make sure this will work on both unix and windows. Is there a way we can rewrite this to be compatible with both? @DAddYE? any ideas

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Oct 28, 2011

Member

Mmm, I've haven't windows but:

File.join('sqlite:/', 'foo', 'bar')

Should do the trick? Any windows user there?

Member

DAddYE commented Oct 28, 2011

Mmm, I've haven't windows but:

File.join('sqlite:/', 'foo', 'bar')

Should do the trick? Any windows user there?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Oct 28, 2011

Member

No, I think can't work, btw I found for sequel this:

Sequel.postgres(:host=>'localhost', :user=>'user', :password=>'password', :database=>'blog')

we have same problem with other adapters?

Member

DAddYE commented Oct 28, 2011

No, I think can't work, btw I found for sequel this:

Sequel.postgres(:host=>'localhost', :user=>'user', :password=>'password', :database=>'blog')

we have same problem with other adapters?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Oct 28, 2011

Member

Im seeing that on windows with access // will work:

Sequel.connect('ado://user:password@server/database?host=server%5cdb_instance&provider=SQLNCLI10')
Member

DAddYE commented Oct 28, 2011

Im seeing that on windows with access // will work:

Sequel.connect('ado://user:password@server/database?host=server%5cdb_instance&provider=SQLNCLI10')
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 19, 2011

As DAddYe mentioned:

Sequel.postgres(:host=>'localhost', :user=>'user', :password=>'password', :database=>'blog')

works for me in Windows, too.

ghost commented Nov 19, 2011

As DAddYe mentioned:

Sequel.postgres(:host=>'localhost', :user=>'user', :password=>'password', :database=>'blog')

works for me in Windows, too.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 4, 2013

Contributor

Should we refactor the database.rb generator for sequel then to include @DAddYE's suggestion?

Contributor

dariocravero commented Jan 4, 2013

Should we refactor the database.rb generator for sequel then to include @DAddYE's suggestion?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 10, 2013

Member

Moving to 0.11.1 for now but let's fix this soon

Member

nesquena commented Mar 10, 2013

Moving to 0.11.1 for now but let's fix this soon

dariocravero pushed a commit that referenced this issue May 16, 2013

Darío Javier Cravero
Fixes #698 by adding an extra "/" to the connection string when using
sqlite3 with Sequel.

It doesn't really hurt Linux/Mac since that extra "/" doesn't really
count at all.
@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero May 16, 2013

Contributor

@nesquena @DAddYE merge #1290 if you're ok with it. It won't work on Windows otherwise :( (just tested it, what a pain really!.. haha). The alternative is to include a commented out version for it for Windows users.

Contributor

dariocravero commented May 16, 2013

@nesquena @DAddYE merge #1290 if you're ok with it. It won't work on Windows otherwise :( (just tested it, what a pain really!.. haha). The alternative is to include a commented out version for it for Windows users.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena May 16, 2013

Member

Looks good to me

Member

nesquena commented May 16, 2013

Looks good to me

WaYdotNET added a commit to WaYdotNET/padrino-framework that referenced this issue May 18, 2013

Merge remote-tracking branch 'upstream/master' into refresh-admin
* upstream/master:
  Skip 1.8.7 test as it doesn't support fetching the encoding on a string
  Fixes padrino#698 by adding an extra "/" to the connection string when using sqlite3 with Sequel.
  Added a test to ensure that params inside an action are properly encoded in UTF-8. This fixes padrino#210 if all tests pass.
  Better path after codereview with @dariocravero
  Typo fix
  Add :file option for delivering mail
  dm:create and dm:drop did not pass the arguments to the #system correctly.  Using the array approach that is being used for the mysql adapter.
  Fixes padrino#1240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment