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

add nimib to important packages #20697

Merged
merged 5 commits into from Feb 9, 2023
Merged

add nimib to important packages #20697

merged 5 commits into from Feb 9, 2023

Conversation

ringabout
Copy link
Member

It is a great library. There is an impressive talk regarding its usage.

@pietroppeter
Copy link
Collaborator

pietroppeter commented Oct 31, 2022

thanks for the nice words and honored to be considered in important packages!

I guess it make sense for us to add devel to CI, we will do that (see pietroppeter/nimib#149)

as mentioned in discord #science chat, toml_serialization (reponsible for current failing checks, as I understand) is a dependency from status, so I will have to follow up with them.

for reference, I am noting also here elements of the discussion in discord #science chat:

  • reported error:
    /home/runner/.nimble/pkgs/toml_serialization-0.2.3/toml_serialization/types.nim(172, 25) template/generic instantiation of `==` from here
      /home/runner/work/Nim/Nim/lib/system/comparisons.nim(309, 6) Error: '==' can have side effects
      > /home/runner/work/Nim/Nim/lib/system/comparisons.nim(334, 13) Hint: '==' calls `.sideEffect` '=='
      >> /home/runner/.nimble/pkgs/toml_serialization-0.2.3/toml_serialization/types.nim(144, 6) Hint: '==' called by '=='
    
  • devel has enabled stricteffects, it seems that it is necessary to annotate the proc in toml_serialization with noSideEffect. ref fixes #19162; enable strictEffects for v2 #19380
  • Does stricteffects break sideeffect analysis?

@pietroppeter
Copy link
Collaborator

C:\Users\ppeterlongo\Documents\toml_serialization\toml_serialization.nim(112, 15) template/generic instantiation of `readValue` from here
C:\Users\ppeterlongo\.nimble\pkgs\serialization-0.1.0\serialization.nim(29, 9) template/generic instantiation of `readValue` from here
C:\Users\ppeterlongo\Documents\toml_serialization\toml_serialization\reader.nim(416, 29) template/generic instantiation from here
C:\Users\ppeterlongo\Documents\toml_serialization\toml_serialization\reader.nim(430, 15) Error: parseValue(r.lex, value) can raise an unlisted exception: Exception
  • not sure how to proceed, help appreciated. @ringabout ?

@ringabout
Copy link
Member Author

ringabout commented Nov 1, 2022

The other issue can be fixed as status-im/nim-toml-serialization#55

@Varriount
Copy link
Contributor

Looks as though this is still failing on Linux and Mac OS.

@pietroppeter
Copy link
Collaborator

Well the PR on toml serialization was merged with failing checks. I assume the maintainers of that package will take over the fix (I was kind of stuck but there was some suggestions, I would have probably moved much slower than them). Until the issue is actually solved (green CI on toml-serialization), I do not expect this to go green.

@pietroppeter
Copy link
Collaborator

The blocker for this is currently status-im/nim-serialization#45

@pietroppeter
Copy link
Collaborator

blocker was fixed (and nim serialization does not have release so it should be already picked up as a dependency) so we could try running again the CI

@pietroppeter
Copy link
Collaborator

I actually expect this to fail again. we have set up devel CI in nimib (pietroppeter/nimib#169) and there is now a new reason for failure (not related to toml-serialization). We will look into it and update here when solved.

@HugoGranstrom
Copy link
Contributor

The same toml problem still exists after fixing the new error (which was caused by #21326): see error logs. So it seems like the PR didn't fix our problem.

@pietroppeter
Copy link
Collaborator

with the latest nimib release we should be good to go for this (you could rerun checks): https://github.com/pietroppeter/nimib/releases/tag/v0.3.6

@pietroppeter
Copy link
Collaborator

(wow so fast!)

@ringabout
Copy link
Member Author

(wow so fast!)

Yeah, I have been watching the progress of this issue, thanks for your diligent work!

@pietroppeter
Copy link
Collaborator

thanks for your diligent work!

it's all thanks to @HugoGranstrom! 👏

@ringabout ringabout merged commit be49126 into devel Feb 9, 2023
@ringabout ringabout deleted the ringabout-patch-1 branch February 9, 2023 12:00
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from be49126

Hint: mm: orc; opt: speed; options: -d:release
166065 lines; 8.101s; 611.098MiB peakmem

c-blake pushed a commit to c-blake/Nim that referenced this pull request Feb 10, 2023
r

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
r

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
r

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
r

Co-authored-by: Clay Sweetser <Varriount@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants