Automatic viz migration on builder-web startup #111

Merged
reynir merged 95 commits from automatic-viz-migration into main 2 months ago
reynir commented 3 months ago
Owner

As #110 but rebased on top of main.

As #110 but rebased on top of main.
reynir added 19 commits 3 months ago
reynir added 1 commit 3 months ago
reynir added 1 commit 3 months ago
rand added 33 commits 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
reynir force-pushed automatic-viz-migration from 468c3c6186 to 6ce71401bf 3 months ago
hannes reviewed 3 months ago
String.(sub dir_str
(length viz_typ_str)
(length dir_str - length viz_typ_str))
|> int_of_string
Poster
Owner

this should be guarded since it may fail (or use int_of_string_opt) -- maybe a warning message should be output?

this should be guarded since it may fail (or use int_of_string_opt) -- maybe a warning message should be output?
rand marked this conversation as resolved
hannes reviewed 3 months ago
lib/model.ml Outdated
and job = job.name
and platform = job.platform
and `Hex sha256 = Hex.of_cstruct main_binary.sha256
in
Poster
Owner

I don't understand the changes in model.ml above this comment -- do they include any change in functionality? could they be reverted from this PR to keep its size small?

I don't understand the changes in model.ml above this comment -- do they include any change in functionality? could they be reverted from this PR to keep its size small?
rand commented 3 months ago
Poster
Owner

It was primarily to keep track of the correct values by renaming p => main_binary, and using it directly instead of binding its contents, again for safety/readability.

It was primarily to keep track of the correct values by renaming p => main_binary, and using it directly instead of binding its contents, again for safety/readability.
rand commented 3 months ago
Poster
Owner

This diff needs to be looked at again after other related changes

This diff needs to be looked at again after other related changes
hannes reviewed 3 months ago
Poster
Owner

could these extra new lines be removed? :)

could these extra new lines be removed? :)
rand marked this conversation as resolved
hannes reviewed 3 months ago
lib/model.ml Outdated
[ "--build-time=" ^ time ; "--sha256=" ^ sha256 ; "--job=" ^ job ;
Poster
Owner

the above debug-binary and opam-switch are no longer used by the scripts, and can be removed.

the above debug-binary and opam-switch are no longer used by the scripts, and can be removed.
rand marked this conversation as resolved
rand added 40 commits 3 months ago
rand added 2 commits 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 2 commits 3 months ago
rand added 1 commit 3 months ago
rand added 2 commits 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand changed title from WIP: Automatic viz migration on builder-web startup to Automatic viz migration on builder-web startup 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
reynir reviewed 3 months ago
reynir left a comment

Only some minor comments. Should we merge? :)

Ok ()
end else
let args =
[ "--cache-dir=" ^ Fpath.to_string cachedir;
Poster
Owner

At some point we should sanitize this, but it can wait until this is merged.

At some point we should sanitize this, but it can wait until this is merged.
Poster
Owner

We could maybe use Filename.quote_command

We could maybe use `Filename.quote_command`
rand commented 3 months ago
Poster
Owner

Below these lines is the line:

|> List.map (fun s -> "\"" ^ String.escaped s ^ "\"")

.. and I actually took this from model.ml, so both places need fixing if String.escaped is not enough

Below these lines is the line: ```ocaml |> List.map (fun s -> "\"" ^ String.escaped s ^ "\"") ``` .. and I actually took this from `model.ml`, so both places need fixing if `String.escaped` is not enough
rand marked this conversation as resolved
for ((V = 1 ; V < $LATEST_TREEMAPVIZ_VERSION ; V++)); do
DIR_REMOVE="${CACHE_DIR}/treemap_${V}"
if rm -r ${DIR_REMOVE}; then
Poster
Owner

Perhaps we can test if $DIR_REMOVE exists first. Then we don't get an error message from rm:

if test -d "$DIR_REMOVE" && rm -r "$DIR_REMOVE"
Perhaps we can test if `$DIR_REMOVE` exists first. Then we don't get an error message from rm: ```shell if test -d "$DIR_REMOVE" && rm -r "$DIR_REMOVE" ```
rand marked this conversation as resolved
hannes reviewed 3 months ago
HASH="$(get_hash)"
echo "$HASH"
Poster
Owner

is this script used? intentionally added? I should get the data-dir as parameter instead of hardcoding a full path...

is this script used? intentionally added? I should get the data-dir as parameter instead of hardcoding a full path...
rand marked this conversation as resolved
hannes reviewed 3 months ago
done
[ -z "$DATA_DIR" ] && die "The --data-dir option must be specified"
Poster
Owner

IIRC we discussed that this script should do a simple sqlite3 query here to bail out early if the database is not present / sqlite3 is not installed / the database does not include the interesting data (should the database version be checked as well (and maybe require something greater than 16 (the uuid-as-string migration))?

IIRC we discussed that this script should do a simple sqlite3 query here to bail out early if the database is not present / sqlite3 is not installed / the database does not include the interesting data (should the database version be checked as well (and maybe require something greater than 16 (the uuid-as-string migration))?
rand commented 3 months ago
Poster
Owner

Yes, that makes sense to do now where set -e is removed. And I agree that a look at db-version makes sense too

Yes, that makes sense to do now where `set -e` is removed. And I agree that a look at db-version makes sense too
rand marked this conversation as resolved
hannes reviewed 3 months ago
fi
if [ ! -e ${CACHE_DIR} ]; then
info "Cache directory '$CACHE_DIR' doesn't exist, so it will be made"
mkdir ${CACHE_DIR}
Poster
Owner

use mkdir -p here to create missing directories in the path as well?

use `mkdir -p` here to create missing directories in the path as well?
rand commented 3 months ago
Poster
Owner

I'm not sure I feel it's better - as user will probably not pass a non-existing nested hierarchy as cache-dir. If user does that - then it can often be a user-error. And as it's easily fixed by user when getting error, then I'd like to keep as is

I'm not sure I feel it's better - as user will probably not pass a non-existing nested hierarchy as cache-dir. If user does that - then it can often be a user-error. And as it's easily fixed by user when getting error, then I'd like to keep as is
reynir marked this conversation as resolved
hannes reviewed 3 months ago
err "Unknown option: '${OPT}'"
usage
#warn "Unknown option: '${OPT}'"
# usage
Poster
Owner

these bottom two cases now both do nothing, so maybe condense them to a single case without commented out commands. and eventually explain "hook scripts have the uniform arguments, thus the main binary is ignored" or something along the lines?

these bottom two cases now both do nothing, so maybe condense them to a single case without commented out commands. and eventually explain "hook scripts have the uniform arguments, thus the main binary is ignored" or something along the lines?
rand commented 3 months ago
Poster
Owner

My update to these lines ok?

My update to these lines ok?
rand marked this conversation as resolved
Owner

Would you mind to look into warnings/errors from https://www.shellcheck.net/ for all the shell scripts? It is picky, but helps a lot esp. when these shell scripts should be executed on different OS with different /bin/sh implementations.

Would you mind to look into warnings/errors from https://www.shellcheck.net/ for all the shell scripts? It is picky, but helps a lot esp. when these shell scripts should be executed on different OS with different /bin/sh implementations.
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
reynir reviewed 3 months ago
[ ! -e "$DB" ] && die "The database doesn't exist: '$DB'"
get_db_version () {
sqlite3 "${DB}" <<EOF
Poster
Owner

You can specify the query at the command line: sqlite3 "$DB" 'PRAGMA user_version;'

While we're at it we should check application_id is 1234839235 as well:

test 1234839235 -eq "$(sqlite3 "$DB" 'PRAGMA application_id;')"

You can specify the query at the command line: `sqlite3 "$DB" 'PRAGMA user_version;'` While we're at it we should check `application_id` is 1234839235 as well: `test 1234839235 -eq "$(sqlite3 "$DB" 'PRAGMA application_id;')"`
rand marked this conversation as resolved
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
rand added 1 commit 3 months ago
Owner

Ready to merge?

Ready to merge?
reynir added 2 commits 2 months ago
reynir merged commit 09a180c3cd into main 2 months ago
hannes deleted branch automatic-viz-migration 2 months ago
The pull request has been merged as 09a180c3cd.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.