Skip to content

Conversation

@kivkiv12345
Copy link
Contributor

@kivkiv12345 kivkiv12345 commented Dec 13, 2024

With the recently added feature of the __FILE_DIR__ environment variable, it becomes apparent that CSH versions from before this commit will not work with scripts requiring the variable to be present.
This PR introduces a feature to help with such scenarios.

The new slash command: "require version csh" is useful in .csh scripts (init or not), where it can tell CSH to: exit, error or warn when a version constraint fails.

At the time of writing, the version constraint argument supports the typical comparisons: "==", "!=", ">=", "<=", ">" and "<".
But I have yet to add support for wildcard constraints, which would be useful for the patch version.

The name of the command is a bit clunky, as the intention is to also allow a command to check APM versions in the future.
Of course, if there are better suggestions for the name before merging, I would be happy to hear them.
The same can be said for the current error-level names: "fatal", "error" and "warn", where we may want to rename "fatal" to quit, so the all represent the resulting action.
I would like for the first letter of the error-level words to be different, as we currently allow single letters for short notation, of course this can also be debated.

"require version csh" is useful in .csh scripts (init or not),
where it can tell CSH to: exit, error or warn when a version constraint fails.
@kivkiv12345 kivkiv12345 self-assigned this Dec 13, 2024
@kivkiv12345
Copy link
Contributor Author

Alright, this PR should be ready now (with wildcard support, unit tests and all). All we may want to do is add some more fail-cases for compare_version().

@kivkiv12345
Copy link
Contributor Author

kivkiv12345 commented Dec 16, 2024

Just noticed that Valgrind compains an initialized value being passed to sscanf() here:

sscanf(constraint, "%d.%d.%d", &constraint_version.major, &constraint_version.minor, &constraint_version.patch);
, which is probably the constraint argument.
Either way it doesn't make much sense since both constraint and constraint_version are very much zero initialized:

csh/src/require_version.c

Lines 106 to 108 in ae87ba1

char constraint[CONSTRAINT_MAXLEN] = {0};
version_t constraint_version = {0, 0, 0};

Copy link
Contributor

@jeanbaptistelab jeanbaptistelab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@troelsjessen
Copy link
Contributor

Let us get it on master

We also no longer use a mutable copy of the version string,
so we allow much larger versions.
@kivkiv12345 kivkiv12345 merged commit f57eeae into master Jan 2, 2025
2 checks passed
@kivkiv12345 kivkiv12345 deleted the PLT-294-require-version-command branch January 2, 2025 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants