Skip to content
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

Make tests pass when --disable-decnum #3120

Merged
merged 1 commit into from
May 15, 2024

Conversation

nicowilliams
Copy link
Contributor

make check fails when --disable-decnum is used. This PR makes make check pass in that case, though at the price of uglifying the manual a bit because the decnum-dependent mantests in the manual need to use a new builtin in a conditional expression to select an expected result.

@nicowilliams nicowilliams self-assigned this May 14, 2024
@@ -1744,6 +1753,8 @@ BINOPS
{f_now, "now", 1},
{f_current_filename, "input_filename", 1},
{f_current_line, "input_line_number", 1},
{f_have_decnum, "have_decnum", 1},
{f_have_decnum, "have_literal_numbers", 1},
Copy link
Member

Choose a reason for hiding this comment

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

If we want to pollute the namespace less we could have one function or a binding that is an object with various build configuration stuff? could also move --build-configuration there. Ex something like jq -n buildconf -> {"decnum": true, ..., "args:" "--disable-debug..."}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have $JQ_BUILD_CONFIGURATION!

: ; jq -cn '$JQ_BUILD_CONFIGURATION'
"--prefix=/tmp/jq --with-oniguruma=builtin --enable-maintainer-mode --srcdir=.. 'CFLAGS=-g -ggdb3 -gdwarf-4 -O0 -pthread' --disable-decnum --enable-docs"
: ; 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, $JQ_BUILD_CONFIGURATION is not an object. It's a string, and while one could parse it, that would be a bit gross, though at least ./config.status --config emits something that is easy enough to parse. E.g., ./configure --prefix=/tmp/jq --with-oniguruma=builtin --enable-maintainer-mode --srcdir=.. CFLAGS="-g -ggdb3 -gdwarf-4 -O0 -pthread" --disable-decnum --enable-docs becomes --prefix=/tmp/jq --with-oniguruma=builtin --enable-maintainer-mode --srcdir=.. 'CFLAGS=-g -ggdb3 -gdwarf-4 -O0 -pthread' --disable-decnum --enable-docs in the output of ./config.status --config.

Anyways, to answer your question, we still need some short-cut for the manual page examples, and what I have now there is rather ugly. The only way I can think of to do better would be to add some sort of decoration in the manual to indicate that some test is only expected to pass if USE_DECNUM or only expected to pass if !USE_DECNUM. WHat would that decoration look like? Ideas?

I'd like to push this and then maybe look for ways to make the mantests in the manual neater.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already split the manual test cases to man.test and manonig.test. We do so by matching the programs, but we can also by adding a field to the manual file indicating the feature support. For the purpose of making test pass without decnum support, that's enough. If there is anyone really want to switch their filters based on the decnum support availability, builtin function will be useful.

@nicowilliams
Copy link
Contributor Author

Why aren't the GitHub Actions running? Just because of the "conflict" in jq.1.prebuilt?

@nicowilliams nicowilliams force-pushed the fix_make_check--disable-decnum branch from 1b6f077 to 802d456 Compare May 15, 2024 17:40
@nicowilliams
Copy link
Contributor Author

Rebased.

@nicowilliams nicowilliams merged commit 6d02d53 into master May 15, 2024
30 checks passed
@nicowilliams nicowilliams deleted the fix_make_check--disable-decnum branch May 15, 2024 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants