refactor: properly report more error types from dep solver

[andrius-puksta-sensmetry]

UPDATE: the description is outdated and there are more changes.

Note for reviewers: this is best reviewed commit-by-commit, since commits are largely self-contained.

Changes:

• Remove version constraints from lockfile (also bump lockfile version to 0.3). They are not needed for version resolution and arguably hurt readability a bit. We may want to still include versions (but not constraints) if we ever add support to depend on two different versions of same package.
  Example lockfile fragment before:

[[project]]
name = "test"
version = "0.0.1"
usages = [
    { resource = "urn:kpar:sysmod", version_constraint = "^5.0.0-alpha" },
]
sources = [
    { editable = "." },
]
checksum = "..."

Example after:

[[project]]
name = "test"
version = "0.0.1"
usages = [
    "urn:kpar:sysmod",
]
sources = [
    { editable = "." },
]
checksum = "..."

• exclude benches and doctests from cargo test, since we have none. This cleans up cargo test output.
• don't include same project (here "same" means specifically same name, version and checksum; it would be better to check some sort of canonical identifier instead of name if/when we have it) more than once in lockfile. Part of Detect and warn or fail in the presence of potentially conflicting projects #9.
• (debug builds only) validate lockfile upon generating it

Fixes

Report a proper error when package was not found:

$ sysand add aaa:bbb
error: unable to select version of `aaa:bbb`: IRI is not resolvable: no resolver was able to resolve the IRI

instead of previous:

$ sysand add aaa:bbb
      Adding usage: `aaa:bbb`
error: Failed to satisfy usage constraints:
requested project(s) alternative nr 0 depends on aaa:bbb

Error message could be improved, but at least now it's explicitly checked and handled.

Report a proper error when required package version was not found:

$ sysand add urn:kpar:sysmod 5.0.0
error: failed to retrieve project(s): requested version unavailable: project `urn:kpar:sysmod`
was found, but the requested version constraint `^5.0.0`
was not satisfied by any of the found versions:
`5.0.0-alpha.2`, `5.0.0-alpha.1`

instead of previous:

$ sysand add urn:kpar:sysmod 5.0.0
      Adding usage: `urn:kpar:sysmod` 5.0.0
error: Failed to satisfy usage constraints:
requested project(s) alternative nr 0 depends on urn:kpar:sysmod no valid alternatives

Fixes #108.

Use Debug impls of reqwest(_middleware) errors to get all error details, since Display impls give no details. This makes errors look uglier. Current:

$ sysand env install aaa:bbb --index=http://localhost:8080/
error: unable to select version of `aaa:bbb`: resolution error: error making an HTTP request:
Reqwest(
    reqwest::Error {
        kind: Request,
        url: "http://localhost:8080/1b45c84078cf7810b6604c7b01169f668e41fd1784c21352e9e89d8833097dd2/versions.txt",
        source: hyper_util::client::legacy::Error(
            Connect,
            ConnectError(
                "tcp connect error",
                127.0.0.1:8080,
                Os {
                    code: 61,
                    kind: ConnectionRefused,
                    message: "Connection refused",
                },
            ),
        ),
    },
)

Previous:

$ sysand env install aaa:bbb --index http://localhost/
error: Choosing a version for aaa:bbb failed
Choosing a version for aaa:bbb failed
error making an HTTP request to 'http://localhost/1b45c84078cf7810b6604c7b01169f668e41fd1784c21352e9e89d8833097dd2/versions.txt':
error sending request for url (http://localhost/1b45c84078cf7810b6604c7b01169f668e41fd1784c21352e9e89d8833097dd2/versions.txt)

Other fixes:

• sysand add: run package and dependency resolver before adding the requested package to .project.json

Refactor

• optimize sysand sync to not re-read lockfile it just generated

Deps

• update logos to 0.16; this version changes semantics of regexes to work properly and (probably) no longer be too greedy. This PR does not change our usage of logos, asidde from fixing line comment parsing.
• update all other Rust deps to latest versions

[tilowiklundSensmetry]

It's worth doing a before and after test on the standard library + official examples to check for regressions. When I initially wrote the lexer I found a lot of weird corner cases especially in the official sysml examples.

[tilowiklundSensmetry]

@andrius-puksta-sensmetry Other than doing a regression test for the lexer update it looks good.

[andrius-puksta-sensmetry]

Ran into a problem/bug: std packages have dependency cycles, and it's unclear how to deal with this when cloning one of them:

• IRI of current (i.e. editable) project is not included in lockfile
• the PR implements lockfile deduplication based on (name, version, checksum) tuple, i.e. projects matching any already included project are not included again
• there is a dependency cycle between current project and its usages (this is the case with e.g. urn:kpar:function-library)

And so invalid lockfile is generated, which does have all needed projects, but the current project does not have an identifier and thus there is an unsatisfied dependency.

Ways to deal with this, none perfect:

1. disallow dependency cycles and fail whenever this is detected. Std uses this, so this probably should be supported
2. add an identifier to editable projects if we know it from context, such as:

• we cloned a project from IRI
• project is part of workspace and has an IRI in .workspace.json

3. don't deduplicate the lockfile (current behavior). This will cause symbol clashes for any non-std dependency cycles and only works for std because we by default don't include symbols for (dependency) std libs in lockfile

This is unsolvable in the general case, since current project has no identifier.

I'll implement 2, since it's better than nothing. Any better ideas?

[andrius-puksta-sensmetry]

Added IRIs for current project to lockfile when the IRIs are known.

[victor-linroth-sensmetry]

• Remove version constraints from lockfile (also bump lockfile version to 0.3). They are not needed for version resolution and arguably hurt readability a bit.

Agree on the readability. It would be nice for the version constraint compatibility to be checked somewhere though, like maybe when syncing?

[andrius-puksta-sensmetry]

It would be nice for the version constraint compatibility to be checked somewhere though, like maybe when syncing?

This would (I think) require resolving all deps again to be able to check transitive deps, while also pinning the already known deps from lockfile to their known versions while resolving.
Maybe this could be behind an optional flag, since resolver can be slow?

Or maybe the lockfile should just include version constraints... What do you think @tilowiklundSensmetry?

[tilowiklundSensmetry]

It would be nice for the version constraint compatibility to be checked somewhere though, like maybe when syncing?

This would (I think) require resolving all deps again to be able to check transitive deps, while also pinning the already known deps from lockfile to their known versions while resolving. Maybe this could be behind an optional flag, since resolver can be slow?

Or maybe the lockfile should just include version constraints... What do you think @tilowiklundSensmetry?

It's not clear to me when this would be important.

If we trust the lockfile has been properly generated, why would versions not match?

If we don't trust the lockfile, why would we trust the versions it claims are correct?

In my mind the main reason to include the version numbers are for better error messages. In case there's a mismatch between what is actually fetched (when syncinc) and what's in the lockfile we can say "Found version X but expected version Y" instead of "Found hash ABCBACBABCABCABC intead of hash DEFEDEFEDEFEDEF".

[andrius-puksta-sensmetry]

In my mind the main reason to include the version numbers are for better error messages. In case there's a mismatch between what is actually fetched (when syncinc) and what's in the lockfile we can say "Found version X but expected version Y" instead of "Found hash ABCBACBABCABCABC intead of hash DEFEDEFEDEFEDEF".

Version numbers remain untouched; this is about version constraints.

[tilowiklundSensmetry]

In my mind the main reason to include the version numbers are for better error messages. In case there's a mismatch between what is actually fetched (when syncinc) and what's in the lockfile we can say "Found version X but expected version Y" instead of "Found hash ABCBACBABCABCABC intead of hash DEFEDEFEDEFEDEF".

Version numbers remain untouched; this is about version constraints.

Right, sorry. My other point is still valid though.

[andrius-puksta-sensmetry]

If we trust the lockfile has been properly generated, why would versions not match?

If we don't trust the lockfile, why would we trust the versions it claims are correct?

Good point. We already discourage editing the lockfile and by definition do trust it. Including constraints would only catch cases where user/sysand/some other tool modified versions of some projects in lockfile to incompatible ones, which is a rather unlikely scenario and one we don't need to handle well (assuming that sysand itself does not generate such lockfiles, which should be ensured).

[victor-linroth-sensmetry]

If we don't trust the lockfile, why would we trust the versions it claims are correct?

It wasn't about checking correctness as much as it was about checking internal consistency. Ideally we'd check correctness too of course.

We already discourage editing the lockfile and by definition do trust it.

I'd say it's an external artifact loaded from outside of the application, so by definition it's untrustworthy. This is as opposed to something inside application having enforced invariants (ideally enforced by the type system).

Including constraints would only catch cases where user/sysand/some other tool modified versions of some projects in lockfile to incompatible ones, which is a rather unlikely scenario and one we don't need to handle well

From my understanding the plan has been to have the lockfile be an open standard and let other tools generate them as well if they wish. May not happen for a while though.

(assuming that sysand itself does not generate such lockfiles, which should be ensured).

And one way of ensuring this would be to have the functionality to validate the lockfiles that sysand generates. As I wrote in the original design:

Currently the lockfile generation process is not that complex, but as it gets more complex it can be good to have a validation step at the end to catch bugs early.

[andrius-puksta-sensmetry]

I'd say it's an external artifact loaded from outside of the application, so by definition it's untrustworthy. This is as opposed to something inside application having enforced invariants (ideally enforced by the type system).

Yes, but lockfile/project info is untrusted only to the extent that bad data must not crash the application. Some validation is required to ensure this, but further validation is only for better diagnostics. In my opinion, it's not worth including constraints just for validation, as incorrect versions cannot (AFAIK) cause crashes.

And one way of ensuring this would be to have the functionality to validate the lockfiles that sysand generates.

This PR includes some form of this: in debug builds it validates the lockfile in canonicalize(), which is always called before writing lock to file. Also symbols name collisions will always be caught when generating the lock structure itself.