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

Merge old jqlang/jq master to the latest #2596

Merged
merged 22 commits into from
Jun 1, 2023
Merged

Conversation

owenthereal
Copy link
Member

After the transfer from stedolan/jq to jqlang/jq, some commits are missing that divert from the older master of stedolan/jq. This is to pull them in: #2594 (comment)

@owenthereal
Copy link
Member Author

cc: @stedolan @nicowilliams

@stedolan
Copy link
Contributor

Something seems to be wrong with the github action definitions here: the windows one failed with an error and the unix ones have hung.

@itchyny
Copy link
Contributor

itchyny commented May 29, 2023

The action workflow does not trigger because the runner ubuntu-18.04 is unavailable since 2022-12-01 April 2023 (ref). Let's use ubuntu-22.04, or ubuntu-latest.

@owenthereal owenthereal force-pushed the old-jqlang-master branch 5 times, most recently from 51bb85b to 9c4ceae Compare May 29, 2023 17:34
@owenthereal
Copy link
Member Author

@stedolan @itchyny @nicowilliams I got the Linux builds going with no problems. But the macos build failed with https://github.com/jqlang/jq/actions/runs/5114162555/jobs/9194126677#step:6:3160. Do you have any insight into what goes wrong? The Linux clang builds ran okay, though.

@wader
Copy link
Member

wader commented May 29, 2023

That is a bit strange, error seems to be:

posix.c:94:3: error: implicit declaration of function 'onig_end' is invalid in C99 [-Werror,-Wimplicit-function-declaration]

There is an issue related to it #2381 which seems to indicate it has been fixed in newer version of oniguruma. But why did the linux build not complain about it? i would have guessed linux (with gcc?) would complain and fail also on -Wimplicit-function-declaration?

@wader
Copy link
Member

wader commented May 29, 2023

Linux build also complains:

posix.c:94:3: warning: implicit declaration of function 'onig_end' is invalid in C99 [-Wimplicit-function-declaration]

But does not use -Werror for some reason. Both linux and macos build uses clang btw.

MacOS build [fails](https://github.com/jqlang/jq/actions/runs/5114162555/jobs/9194126677#step:6:3160) due to

```
posix.c:94:3: error: implicit declaration of function 'onig_end' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
  onig_end();
```

The current `oniguruma` revision
6fa38f4084b448592888ed9ee43c6e90a46b5f5c (15 Mar 2017) lacks the following explicit declaration in src/onigposix.h:

```
ONIG_EXTERN int onig_end P_((void));
```

This was added to oniguruma in revision 5a24a49d710a9e3bb8ff11d12e1eae5a9f9df40c (8 Sep 2017).

Ref: #2381
@owenthereal
Copy link
Member Author

@wader:

There is an issue related to it #2381 which seems to indicate it has been fixed in newer version of oniguruma

Thank you! It's an issue with oniguruma's missing onig_end function in the header! I pinned oniguruma to the suggested revision 5a24a49d710a9e3bb8ff11d12e1eae5a9f9df40c as indicated in #2381, and the macos builds are now passing! Weirdly, the Linux clang build is warning while the macos clang build is erroring...

@wader
Copy link
Member

wader commented May 29, 2023

@owenthereal Some addition info: i can reproduce on macOS 13.3.1 with clang 14.0.3 (clang-1403.0.22.14.1) and the error happens during make check in one of the oniguruma tests.

But i agree that update is probably the best solution, still confused why macOS clang uses -Werror and Linux does not.

@wader
Copy link
Member

wader commented May 30, 2023

A patch like this seems to fix the \r issue but it would be nice to know why it worked before on windows CI?

diff --git a/src/lexer.l b/src/lexer.l
index 6b9164b..28d281e 100644
--- a/src/lexer.l
+++ b/src/lexer.l
@@ -123,7 +123,7 @@ struct lexer_param;
 ([a-zA-Z_][a-zA-Z_0-9]*::)*[a-zA-Z_][a-zA-Z_0-9]*  { yylval->literal = jv_string(yytext); return IDENT;}
 \.[a-zA-Z_][a-zA-Z_0-9]*  { yylval->literal = jv_string(yytext+1); return FIELD;}

-[ \n\t]+  {}
+[ \r\n\t]+  {}

 . { return INVALID_CHARACTER; }

(could possibly also include \f\v which seems to be what gojq do)

@wader
Copy link
Member

wader commented May 30, 2023

It would be great if someone could have a look at the pow/log2 issue, i haven't started yet

@itchyny
Copy link
Contributor

itchyny commented May 30, 2023

I don't be surprised log2(pow2(x)) is not exactly equal to x on computer due to floating-point number precision. We can just round the result or assert the absolute difference is small enough.

@owenthereal owenthereal force-pushed the old-jqlang-master branch 5 times, most recently from a2e87f7 to 2d367c1 Compare May 31, 2023 01:09
@wader
Copy link
Member

wader commented May 31, 2023

I don't be surprised log2(pow2(x)) is not exactly equal to x on computer due to floating-point number precision. We can just round the result or assert the absolute difference is small enough.

Good idea, let's do that. Maybe a mystery for later to figure what differs from the old build env.

@owenthereal sounds good to you?

@wader
Copy link
Member

wader commented May 31, 2023

@itchyny any ideas about the \r issue? just use gitattributes for now or should we fix something? as \n and \r\n do work (would be weird otherwise, but just \r don't) maybe this is limited to the jq --run-tests parser code somehow, haven't checked yet.

@itchyny
Copy link
Contributor

itchyny commented May 31, 2023

Using gitattributes until fixing the lexer sounds good. I think the defaults of core.autocrlf are different between AppVeyor and actions/checkout.

@owenthereal
Copy link
Member Author

I don't be surprised log2(pow2(x)) is not exactly equal to x on computer due to floating-point number precision. We can just round the result or assert the absolute difference is small enough.

Sounds good to me. Does it require some changes to the test and assertion?

Using gitattributes until fixing the lexer sounds good. I think the defaults of core.autocrlf are different between AppVeyor and actions/checkout.

I found actions/checkout#135. I followed the recommendation to turn on LG line endings for all text: 93465d5

@wader
Copy link
Member

wader commented May 31, 2023

Sounds good to me. Does it require some changes to the test and assertion?

Yeap something like this should work i think:

diff --git a/tests/jq.test b/tests/jq.test
index 2d5c36b..9aa435a 100644
--- a/tests/jq.test
+++ b/tests/jq.test
@@ -1565,7 +1565,7 @@ try (1%.) catch .
 jq: error: Division by zero? at <top-level>, line 1:

 # Basic numbers tests: integers, powers of two
-[range(-52;52;1)] as $powers | [$powers[]|pow(2;.)|log2] == $powers
+[range(-52;52;1)] as $powers | [$powers[]|pow(2;.)|log2|round] == $powers
 null
 true

You can run the tests with jq --run-tests < tests/jq.test

@owenthereal
Copy link
Member Author

Weeee....CI is passing finally! Thank you all for the help! Would you happen to have any more feedback? If not, let's merge. At the moment, the CI only compiles & runs tests. A follow-up PR is needed to publish releases to GH Releases.

@wader
Copy link
Member

wader commented Jun 1, 2023

No worries! happy to help and i learned some windows stuff :) Yeap let's merge! the things i'm don't know/are worried about is publish and signing windows binaries (are they built "correctly" etc?).

@owenthereal owenthereal merged commit 4975b01 into master Jun 1, 2023
@owenthereal owenthereal deleted the old-jqlang-master branch June 1, 2023 17:09
@wader
Copy link
Member

wader commented Jun 1, 2023

🥳

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.

5 participants