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

Import std/log environment; CI: Update Ubuntu #103

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Conversation

FMotalleb
Copy link
Contributor

Will fix this issue but its not a good fix anyway

Description

This is a hotfix PR to address an unusual issue with the std log package in Nushell. When the log is imported during the initialization stage, it doesn’t persist long enough to be fully available at runtime, specifically, the environment variables seem to disappear, although the log function itself remains accessible. This is a temporary fix and might not be the final solution. It could potentially be a bug in Nushell, but unfortunately, I don't have the time to investigate it further.

for more information please check this issue

I think this fix changes the output of the nupm, Plz close this PR if you found any real solution

@amtoine
Copy link
Member

amtoine commented Oct 19, 2024

why only add this to nupm install and not the other commands?

@FMotalleb
Copy link
Contributor Author

I didn't knew where to put it, actually, now if you check the CI its broken for the same reason
https://github.com/nushell/nupm/actions/runs/11405030607/job/31741019156?pr=103

@amtoine
Copy link
Member

amtoine commented Oct 19, 2024

mm that looks quite annoying 😅

sounds like something a library shouldn't have to worry about, right?

@fdncred
Copy link
Collaborator

fdncred commented Oct 19, 2024

@NotTheDr01ds thoughts on how to get rid of this error?

@FMotalleb
Copy link
Contributor Author

FMotalleb commented Oct 19, 2024

well after some investigation I found out that when you have a module like this

example.nu

use std/log

export def test_cmd [param: string, ] {
  log info $param
}

then running use example.nu

the imported function is working but the env vars aren't

~/.../test> example test_cmd test                                                                                                         10/19/2024 07:14:36 PM PM
Error: nu::shell::column_not_found

  × Cannot find column 'NU_LOG_FORMAT'
     ╭─[std/log\mod.nu:158:9]
 157 │     let log_format = if ($format_string | is-empty) {
 158 │         $env.NU_LOG_FORMAT
     ·         ─────────┬────────┬
     ·                  │        ╰── value originates here
     ·                  ╰── cannot find column 'NU_LOG_FORMAT'
 159 │     } else {
     ╰────

but

export def test_cmd [param: string, ] {
  use std/log
  log info $param
}

this one works

maybe its a bug in env isolation

@NotTheDr01ds
Copy link

NotTheDr01ds commented Oct 21, 2024

Apologies - missed the notification again. @FMotalleb Thanks for pinning this down - Your MRE showed me what's happening ...

This goes back to nushell/nushell#13403. I'm not sure if it's a bug or design, but importing a module within another module doesn't re-export the parent's export-env block.

The workaround is to make sure the parent module re-exports the child module's (std/log here) environment:

use std/log

export-env {
  use std/log []
}

export def test_cmd [param: string, ] {
  log info $param
}

@NotTheDr01ds
Copy link

NotTheDr01ds commented Oct 21, 2024

I thought about defining these at startup and just taking them out of std/log, but this is going to be a problem every time we add new exported environment variables to a module (std or not). What I don't want to do is actually call std/log's export-env during startup since that will cause the entire submodule to load, increasing startup times. That was one of the problems with the previous implementation.

Technically, it's also a problem for the dirs module, but since that provides user functionality, it's not something that's likely to get imported in another module.

It would be great if the root issue could be fixed, but perhaps we work around it for now by defining those environment variables during startup? The performance impact for that should be minimal.

@NotTheDr01ds
Copy link

sounds like something a library shouldn't have to worry about, right?

Yeah, that was my thought in nushell/nushell#13403

@kubouch
Copy link
Contributor

kubouch commented Oct 23, 2024

If I understand correctly, the problem is that environment variables from std/log are not available right? Then, I think the export-env solution is the best bet. It works like that "by design", but it's not a very good design, lol, I'll elaborate more in the linked issue. For this PR, I'd propose go with the export-env method.

@FMotalleb
Copy link
Contributor Author

well "It Doesn't work on my machine" 😂
I replaced use std log in install subcommand (the only location that std log is imported) with

use std/log

export-env {
    use std/log []
}

I still face the same error (cannot find column 'NU_LOG_FORMAT')

@FMotalleb
Copy link
Contributor Author

oh my bad there was another export-env after my block

@kubouch
Copy link
Contributor

kubouch commented Oct 24, 2024

OK, looks good, thanks!

@kubouch kubouch changed the title fix: import logging package in main body Import std/log environment; CI: Update Ubuntu Oct 24, 2024
@kubouch kubouch merged commit 5ce8940 into nushell:main Oct 24, 2024
3 checks passed
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.

Bug: NU_LOG_FORMAT and NU_LOG_DATE_FORMAT are not defined
5 participants