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

Toggle between drvname/name when pressing n #162

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

picnoir
Copy link
Contributor

@picnoir picnoir commented Nov 22, 2024

Note: I'll add more context when this will be ready to merge. But in very short, sometimes, I'm on my laptop and start a build before realizing I'm about to build something massive. In that case, I'd like to copy paste the drv path to launch the build on a beefy remote builder. The idea is to make NOM print the drv path instead of the derivation name when pressing the n key.

TODO

  • help screen
  • freeze print when pressing f

Copy link
Owner

@maralorn maralorn left a comment

Choose a reason for hiding this comment

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

Nice. I know you are not finished, but I hope it’s okay when I already leave a few remarks.

lib/NOM/IO.hs Outdated
Comment on lines 255 to 257
System.IO.hSetBuffering stdin NoBuffering
System.IO.hSetEcho stdin False
Copy link
Owner

Choose a reason for hiding this comment

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

We should only do this once not within this forever loop. We should also make sure, that we restore the terminal state correctly on a crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped this acquisition/release in a bracket call. PTAL

Comment on lines +200 to +214
data PrintNameStyle = PrintName | PrintDerivationPath deriving stock (Show, Eq, Ord, Generic)

data PrintState = MkPrintState
{ printName :: PrintNameStyle
, printHelp :: Bool
}
deriving stock (Show, Eq, Ord, Generic)

initPrintState :: PrintState
initPrintState =
MkPrintState
{ printName = PrintName
, printHelp = False
}

Copy link
Owner

Choose a reason for hiding this comment

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

I somehow consider this thing more generally "RuntimeConfig" than "PrintState". And I wonder we could even just merge it with "Config". (But that might be annoying depending on where that is used.)

Comment on lines -154 to +157
stateToText :: Config -> NOMV1State -> Maybe (Window Int) -> (ZonedTime, Double) -> Text
stateToText config buildState@MkNOMV1State{..} = memo printWithSize . fmap Window.height
stateToText :: Config -> NOMV1State -> PrintState -> Maybe (Window Int) -> (ZonedTime, Double) -> Text
stateToText config buildState@MkNOMV1State{..} printState = memo printWithSize . fmap Window.height
Copy link
Owner

Choose a reason for hiding this comment

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

This might be a problem. What I was trying to do here is to memoize this whole function. It should only be called new when the state or the window size changes. The function tries to precalculate most of its state and only use the input time event a little. (This might be overengineered and I am not sure whether I ever benchmarked this.)

Never-the-less passing in the new print state on every print event will invalidate the memoisation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I see. Hmm. I'm not familiar with memotrie, I'll have to read a bit about it to understand how it works.

@maralorn
Copy link
Owner

By the way, this is awesome because there is a different feature which I would love to see: I have a commented out section somewhere where download state is shown per output and not per derivation. It is really nice because you actually see which outputs of a derivation you are downloading. However I deemed it to complex to have it on by default, but we can bring it back as a toggleable option.

@picnoir picnoir force-pushed the pic/print-drvs branch 2 times, most recently from a481685 to 13894dd Compare November 22, 2024 21:01
lib/NOM/IO.hs Outdated
let writeToScreen :: IO ()
writeToScreen = writeStateToScreen (not config.silent) linesVar state_var output_builder_var refresh_display_var maintenance printer output_handle
let keepProcessingStdin :: IO ()
keepProcessingStdin = bracket setBuffering restoreBuffering $ const processStdinForeverLoop
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for not being more explicit about this, but we are already doing some terminal mode switching in Main.hs (grep for hideCursor, showCursor). We should strive for a unified solution.

TODO

- help screen
- freeze print when pressing f
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.

2 participants