Command line parsing to get version information

[lyderic]

Following the discussion about getting SQLpage version information from the command line, I used the clap library to provide basic CLI parsing.

As a Go programmer, I looked for an equivalent to Go cobra and clap looked like a good match.

Now both sqlpage --help and sqlpage --version work.

This is my first attempt at rust ever, so I don't fully understand what I did, in particular the use of async fn main() instead of fn main() in main.rs. Please review carefully.

I basically followed one of the examples provided by clap.

[lovasoa]

Hi @lyderic ! Thanks for the contribution ! I'll try to iterate on this a little bit before merging. If we add command-line arguments support, we should probably handle it in app_config.rs, and at least allow setting the web_root from the command line.

[lyderic]

Fully agreed. While we're at it, we could also at a minimum allow port and listen_on to be able to be set from the command line as well (and add more later on possible people's requests).

Do you want me to have a go at it? I guess the first step for me is to understand how app_config.rs works and how to link variables in this file to clap provided variables...

[lovasoa]

If you can do it, that would be great!

[lovasoa]

I'll help you whenever you need

[lyderic]

I figured out how to put the clap related code into app_config.rs.

I could add options for web_root, port, etc.

However I can't figure out how to use the command line values into the app itself i.e. how to include the variables in the 'Cli' struct into the 'AppConfig' struct.

Another thing I am lost with: the 'listen_on' variable is of type 'SocketAddr' and I couldn't get a string representation of it.

I guess a significant refactorisation is needed to have clap providing a default 'AppConfig' struct, I am currently looking at this.

As you closed this PR, shall I open a new one for you to see where I'm at?

[lovasoa]

You closed the pr, not me ! ;) You can reopen it if it was a mistake.

[lovasoa]

I think we should have two different structs, and not offer the exact same options in AppConfig and in the Cli.
The Cli should offer a way to point to a configuration file, a configuration directory, a web root, and set the logging level.

The data from the Cli struct can overwrite the data from the AppConfig here: https://github.com/lovasoa/SQLpage/blob/2146dfbe72a8d0a2249eddc053409ba6fc627828/src/app_config.rs#L151

[lyderic]

Oops sorry, was a mistake indeed ;-) I reopen and remerge.

[lyderic]

I think we should have two different structs, and not offer the exact same options in AppConfig and in the Cli. The Cli should offer a way to point to a configuration file, a configuration directory, a web root, and set the logging level.

Fair enough, so to be clear, you'd like the Cli struct to have these fields:

• web_root PathBuf
• configuration_directory PathBuf
• configuration_file ???
• logging_level ???

I can only find the first two in the AppConfig struct though. So for the moment, I will focus on those (web_root and configuration_directory).

[lyderic]

I found config_file now ;-)

[lovasoa]

Merci @lyderic !

[lyderic]

Merci @lyderic !

De rien ;-) I am afraid I couldn't go as far as I had hoped and I was planning to have another look at it this weekend. I am looking at your changes now and I am learning a lot about rust and SQLpage. Maybe in another future contribution I will be able to provide the full implementation.

[lyderic]

I don't understand everything, but I can compile the code (I think) and it doesn't work. This is a record of what I did:

[user@sqlp SQLpage-lovasoa]$ git branch -avv
* main                                30d993b [origin/main] changelog: cli
  remotes/origin/HEAD                 -> origin/main
  remotes/origin/main                 30d993b changelog: cli
  remotes/origin/test_set_with_dollar 6d78757 test set with dollar
[user@sqlp SQLpage-lovasoa]$ cargo build
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.18s
[user@sqlp SQLpage-lovasoa]$ mkdir ~/asite
[user@sqlp SQLpage-lovasoa]$ cd ~/asite
[user@sqlp asite]$ ~/SQLpage-lovasoa/target/debug/sqlpage --help
sqlpage 0.28.0
Build data user interfaces entirely in SQL. A web server that takes .sql files and formats the query
result using pre-made configurable professional-looking components.

USAGE:
    sqlpage [OPTIONS]

OPTIONS:
    -c, --config-file <CONFIG_FILE>    The path to the configuration file
    -d, --config-dir <CONFIG_DIR>      The directory where the sqlpage.json configuration, the
                                       templates, and the migrations are located
    -h, --help                         Print help information
    -V, --version                      Print version information
    -w, --web-root <WEB_ROOT>          The directory where the .sql files are located
[user@sqlp asite]$ ~/SQLpage-lovasoa/target/debug/sqlpage -w foo
[2024-09-06T06:06:10.262Z INFO  sqlpage::app_config] No DATABASE_URL provided, ./sqlpage/sqlpage.db is writable, creating a new database file.
[2024-09-06T06:06:10.262Z INFO  sqlpage::webserver::database::connect] Connecting to database: sqlite://./sqlpage/sqlpage.db?mode=rwc
[2024-09-06T06:06:10.264Z INFO  sqlpage::webserver::database::migrations] Not applying database migrations because './sqlpage/migrations' does not exist
[2024-09-06T06:06:10.415Z INFO  sqlpage::webserver::http] SQLPage v0.28.0 started successfully.
        Now listening on 0.0.0.0:8080: accessible from the network, and locally on http://localhost:8080
        You can write your website's code in .sql files in foo
^C[2024-09-06T06:06:12.603Z INFO  sqlpage::webserver::database] Closing all database connections...
[2024-09-06T06:06:12.604Z INFO  sqlpage] Server stopped gracefully. Goodbye!
[user@sqlp asite]$ tree -a
.
└── sqlpage
    └── sqlpage.db

2 directories, 1 file

I was expecting a ~/asite/foo directory to get created. I also tried the other switches (-c, -d) and they don't work either.

What am I missing?

[lovasoa]

You have to point the web root to an existing folder containing your .sql files.

[lovasoa]

I added an explicit check, for sqlpage to report the issue on startup if the folder does not exist

[lyderic]

Ok. This still doesn't work for me, although I get the warning now.

[user@sqlp ~]$ ~/SQLpage-lovasoa/target/debug/sqlpage -w asite
[2024-09-06T06:57:32.732Z INFO  sqlpage::app_config] No DATABASE_URL provided, ./sqlpage/sqlpage.db is writable, creating a new database file.
[2024-09-06T06:57:32.733Z ERROR sqlpage] Web root is not a valid directory: "asite"

Now, I create the directory asite in $HOME and I put an index.sql in it:

[user@sqlp ~]$ mkdir asite
[user@sqlp ~]$ ed asite/index.sql
asite/index.sql: No such file or directory
a
SELECT 'text' AS component, 'Hello, world!' AS contents;
.
wq
57
[user@sqlp ~]$ cat asite/index.sql 
SELECT 'text' AS component, 'Hello, world!' AS contents;

sqlpage directory and sqlpage.db are created, but in $HOME, not in $HOME/asite:

[user@sqlp ~]$ ~/SQLpage-lovasoa/target/debug/sqlpage -w asite
[2024-09-06T07:02:48.678Z INFO  sqlpage::webserver::database::connect] Connecting to database: sqlite:///home/unix/sqlpage/sqlpage.db
[2024-09-06T07:02:48.679Z INFO  sqlpage::webserver::database::migrations] Not applying database migrations because './sqlpage/migrations' does not exist
[2024-09-06T07:02:48.831Z INFO  sqlpage::webserver::http] SQLPage v0.28.0 started successfully.
        Now listening on 0.0.0.0:8080: accessible from the network, and locally on http://localhost:8080
        You can write your website's code in .sql files in asite
^C[2024-09-06T07:02:50.821Z INFO  sqlpage::webserver::database] Closing all database connections...
[2024-09-06T07:02:50.825Z INFO  sqlpage] Server stopped gracefully. Goodbye!
[user@sqlp ~]$ tree $HOME/asite $HOME/sqlpage
/home/unix/asite
└── index.sql
/home/unix/sqlpage
└── sqlpage.db

2 directories, 2 files

Does it mean that the whole directory structure needs to be created before using --web-root?

[lovasoa]

I think there is some confusion here:

• The web root is the folder that contains your .sql file. By default, it is set to the current directory (.).
• The configuration directory is the directory that contains sqlpage.json, migrations, and templates. By default, it is set to ./sqlpage (so, by default, it is inside the web root. but it does not have to be, and if you create your own configuration directory, I recommend putting it outside of the web root).
• If the database connection string is not defined in the configuration, it is set to $CONFIGURATION_DIRECTORY/sqlpage.db by default, and created if it does not exist.

So in your example above, everything is working correctly (at least according to the current documented behavior):

• your web root is set to ./asite/
• your config dir is not set, so it defaults to ./sqlpage/
• your database connection string is not set, so it defaults to ./sqlpage/sqlpage.db. Since the file does not exist, it gets created.

Do you think the default behavior should be changed ? The risk is breaking everyone's currently working setup.
Do you think we should update the --help text to better explain this ?

[lyderic]

Ok. It is confusing indeed (at least for me). I was expecting sqlpage and sqlpage.json to be below web-root, as follows:

1. web-root: ~/asite
2. config-dir: ~/asite/sqlpage
3. config-file: ~/asite/sqlpage/sqlpage.json

Now, I cannot get them to be create like this. This doesn't work:

$ ~/SQLpage-lovasoa/target/debug/sqlpage --web-root asite --config-dir asite/sqlpage --config-file asite/sqlpage/sqlpage.json
[2024-09-06T08:48:52.739Z ERROR sqlpage] Configuration file does not exist: "asite/sqlpage/sqlpage.json"

Creating the whole structure before using the options doesn't work either:

$ rm -rf ~/asite ~/sqlpage
$ mkdir -pv asite/sqlpage
mkdir: created directory 'asite'
mkdir: created directory 'asite/sqlpage'
$ echo 'port: 8008' > asite/sqlpage/sqlpage.yaml
$ ~/SQLpage-lovasoa/target/debug/sqlpage --web-root asite --config-dir asite/sqlpage --config-file asite/sqlpage/sqlpage.yaml
[2024-09-06T09:00:01.858Z INFO  sqlpage::app_config] No DATABASE_URL provided, ./sqlpage/sqlpage.db is writable, creating a new database file.
[2024-09-06T09:00:01.859Z INFO  sqlpage::webserver::database::connect] Connecting to database: sqlite://./sqlpage/sqlpage.db?mode=rwc
[2024-09-06T09:00:01.859Z INFO  sqlpage::webserver::database::migrations] Not applying database migrations because 'asite/sqlpage/migrations' does not exist
[2024-09-06T09:00:02.022Z INFO  sqlpage::webserver::http] SQLPage v0.29.0 started successfully.
        Now listening on 0.0.0.0:8008: accessible from the network, and locally on http://localhost:8008
        You can write your website's code in .sql files in asite
^C[2024-09-06T09:00:03.134Z INFO  sqlpage::webserver::database] Closing all database connections...
[2024-09-06T09:00:03.137Z INFO  sqlpage] Server stopped gracefully. Goodbye!
$ tree ~/asite ~/sqlpage
/home/unix/asite
└── sqlpage
    └── sqlpage.yaml
/home/unix/sqlpage
└── sqlpage.db

As you can see ~/sqlpage still gets created...

[lovasoa]

What we could do is set the config directory to $WEB_ROOT/sqlpage instead of ./sqlpage by default. When the web root is not specified, this would result in the same default config dir as in the current setup, so it should not be too disruptive to existing users.

And in the example you give, we definitely have a bug: the configuration directory from the CLI is not taken into account when generating the default database path. It would work if you had set the config directory using the SQLPAGE_CONFIGURATION_DIRECTORY environment variable instead.

[lovasoa]

The source of the bug is here : https://github.com/lovasoa/SQLpage/blob/bf9f03324e1532cd90f7f808d71aa7a8ca45280c/src/app_config.rs#L344-L345

Do you want to have a stab at fixing it ?

[lyderic]

Yes, let's see what I can do :-)

[lyderic]

What we could do is set the config directory to $WEB_ROOT/sqlpage instead of ./sqlpage by default. When the web root is not specified, this would result in the same default config dir as in the current setup, so it should not be too disruptive to existing users.

Yes, I agree. That makes the most sense to me.

And in the example you give, we definitely have a bug: the configuration directory from the CLI is not taken into account when generating the default database path. It would work if you had set the config directory using the SQLPAGE_CONFIGURATION_DIRECTORY environment variable instead.

I confirm it works, but the directory path has to be provided in full, not as a subdir of the web-root, i.e.

SQLPAGE_CONFIGURATION_DIRECTORY=${HOME}/asite/foo

NOT: SQLPAGE_CONFIGURATION_DIRECTORY=foo

So we would have two different behaviours depending on whether we use the CLI option or the environment variable. That doesn't sound good to me.

Maybe we should simplify in the end and only provide --web-root from the CLI, the rest being configured by environment variables or JSON/Yaml entries...

[lovasoa]

we would have two different behaviours depending on whether we use the CLI option or the environment variable

no. What we want to do is move the default database creation logic to after the configuration parameter has been set (by either the cli or the env). This way we will always create the db in the right folder. Currently we create the database at configuration parsing time, this is wrong buggy and confusing.

[lyderic]

Ok. In this case, we should use clap's features. As far as I understand, this library allows to set up env vars AND cli options. I will try to work something out to show you what I have in mind.

[lovasoa]

Our configuration parsing library already parses the env vars, you shouldn't have to do anything on that front. Just move the database creation logic to after the configuration has been fully set.

[lyderic]

Ok, noted

…

On Fri, 6 Sept 2024, 14:12 Ophir LOJKINE, ***@***.***> wrote: Our configuration parsing library already parses the env vars, you shouldn't have to do anything on that front. Just move the database creation logic to after the configuration has been fully set. — Reply to this email directly, view it on GitHub <#565 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGTK5OAW2FCONHRUV2LDH3ZVGLI5AVCNFSM6AAAAABNPXIZE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZTHEYTCMZQG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lyderic]

Hi,

This is what I have worked out so far and I am stuck now. I think the bug is in app_config.rs on line 236:

PathBuf::from("./sqlpage")

./sqlpage is hardcoded, but it should come from the configuration.

I am now trying to find a way to get this value from the AppConfig struct. This is where I am stuck. I don't understand how the AppConfig struct is populated, the difference with the AppConfig impl etc.

Am I in the right direction?

[lovasoa]

Yes, I think you are doing the right thing. AppConfig is initialized in load_from_file, and currently the default database is created during the population of the struct. We should wait until after the AppConfig has been fully initialized, then test whether database_url is empty, and if it is, then create the default database and write its url in app_config.database_url

[lyderic]

I am just learning about all of this works. Rust is quite fascinating. However, it's quite involved as I have to take things from scratch. I think I might be able to fix the bug eventually, but it will take time (in the order of several days as I can't work on this full time). Depending on when you want to release 0.29.0 (and also whether you want to have the CLI functionality in it), I let you decide if you wait for me. I will follow whatever happens.

[lovasoa]

I'll be happy to wait for you. And to answer your questions if you have more !

Even outside of the precise bug you are working on, getting external contributions is healthy for the project. I'll gladly help you go through the finish line on this even if this means taking a little more time.

[lyderic]

Excellent. I come back with questions in due course then.

[lyderic]

I have make progress in rust and I now understand how to debug a little better. Now I'm stuck here: I can't work out how to use the variables of the cli struct.

For example, line 236, I would like to use the config_dir variable instead of the hardcoded "./sqlpage", i.e. do something like:

PathBuf::from(cli.config_dir)

I get an error saying that value cli is not in scope.

How would you do this? Certainly, you wouldn't use a global variable, would you?

[lovasoa]

The problem is here:
https://github.com/lovasoa/SQLpage/blob/35d729b96b02c60a1608787c3f3c8c203b80d54a/src/app_config.rs#L137

We try to set the default database connection string during the initialization of the configuration struct. We should let it be empty by default, and then, one the structure is fully formed, change the empty connection string back to one that is based on the configuration directory.

If you do this after full initialization, you will have access to the configuration directory that is in the struct.

[lyderic]

In order to change the database_url string after the initialization of the AppConfig struct, I need to make the app_config variable mutable after it has been instantiated in main's start() function, right?

So this line:

https://github.com/lovasoa/SQLpage/blob/a2fcc46ea92b72178c4e412a24911f4ac83abdf2/src/main.rs#L17

needs to be:

let mut app_config = app_config::load_from_cli()?;
app_config.database_url = ...

Or is there a better way?

[lovasoa]

I think the best place to mutate it is inside app_config.rs, just before it gets validated here:

https://github.com/lovasoa/SQLpage/blob/a2fcc46ea92b72178c4e412a24911f4ac83abdf2/src/app_config.rs#L48

[lyderic]

Ah yes, of course! Trying this now... Thanks.

[lovasoa]

I hadn't paid enough attention, but the version of clap introduced in this pull request is outdated. I updated it to v4

[lyderic]

Update: I am trying to move the logic of creating the configuration_directory out of the struct initialization. I am having a hard time figuring out the Path, BufPath, &str and String variations of things. Please bare with me while I work this out ;-)

[lyderic]

This is where I'm stuck now:

In order to have config.database_url empty, I initialize database_url with #[serde(default)] instead of #[serde(default = "default_database_url")].

Then, just before config.validate, I add these lines:

config.configuration_directory = cli.config_dir.clone().unwrap_or(PathBuf::from("./sqlpage"));
config.database_url = default_database_url(&config.configuration_directory);

config.validate()?;
...

Then, I amended the default_database_url() function to get the configuration_directory as argument:

fn default_database_url(configuration_directory: &PathBuf) -> String {
    let prefix = "sqlite://".to_owned();
    
    if cfg!(test) {
        return prefix + ":memory:";
    }   
            
    #[cfg(not(feature = "lambda-web"))]
    {
        let config_dir = cannonicalize_if_possible(configuration_directory.as_path());
...

Please refer to my (not working) fork. It compiles, but it seems that the validation takes place before the config.validate() call. Hence the configuration_directory doesn't get created.

I don't understand the validation.

[lovasoa]

Ok, let me take it from here !

[lovasoa]

@lyderic , feel free to test the latest version from the main branch

[lyderic]

Many thanks!!! I hope I was of some help.

All works as expected now:

[user@sqlp ~]$ mkdir -pv foo/bar
mkdir: created directory 'foo'
mkdir: created directory 'foo/bar'
[user@sqlp ~]$ echo 'port: 8008' > foo/bar/baz.yaml
[user@sqlp ~]$ ~/SQLpage-lovasoa/target/debug/sqlpage -w foo -d foo/bar -c foo/bar/baz.yaml
[2024-09-14T15:53:07.018Z INFO  sqlpage::app_config] No DATABASE_URL provided, /home/user/foo/bar/sqlpage.db is writable, creating a new database file.
[2024-09-14T15:53:07.018Z INFO  sqlpage::webserver::database::connect] Connecting to database: sqlite:///home/user/foo/bar/sqlpage.db?mode=rwc
[2024-09-14T15:53:07.019Z INFO  sqlpage::webserver::database::migrations] Not applying database migrations because '/home/user/foo/bar/migrations' does not exist
[2024-09-14T15:53:07.175Z INFO  sqlpage::webserver::http] SQLPage v0.29.0 started successfully.
        Now listening on 0.0.0.0:8008: accessible from the network, and locally on http://localhost:8008
        You can write your website's code in .sql files in foo
^C[2024-09-14T15:53:08.991Z INFO  sqlpage::webserver::database] Closing all database connections...
[2024-09-14T15:53:08.992Z INFO  sqlpage] Server stopped gracefully. Goodbye!
[user@sqlp ~]$ tree foo
foo
└── bar
    ├── baz.yaml
    └── sqlpage.db

Notes:

• The web-root and config-dir directories are not created by default. You have to mkdir them first. Maybe it's worth considering sqlpage doing the full directory structure creation. Not a big deal though.

• config-dir is not a subdirectory of web-root by default. Hence -d foo/bar instead of -d bar. This is a little odd to me (but that's just me, I guess).

[lovasoa]

The config dir is created if necessary, not the web root (it should point to an existing dir where your .sql files are).

Creating a custom config dir inside the web root is dangerous, SQLPage should probably warn you against it. You don't want your configuration file and database to be pubic.

[lyderic]

Creating a custom config dir inside the web root is dangerous, SQLPage should probably warn you against it. You don't want your configuration file and database to be public.

Ah yes of course! Silly me, you're completely right!