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

daml damlc init reads build-options from daml.yaml #16492

Merged
merged 7 commits into from Mar 14, 2023
Merged

Conversation

akrmn
Copy link
Contributor

@akrmn akrmn commented Mar 8, 2023

Closes #14591

(errMsgs, parseResult) = parse args
Command _ _ io <- handleParseResult parseResult
forM_ errMsgs $ \msg -> do
hPutStrLn stderr msg
withProgName "damlc" io

-- | Commands for which we add the args from daml.yaml build-options.
cmdUseDamlYamlArgs :: [CommandName]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not my first time fixing a ticket like this, so I thought this would help find the place to change if we get another one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other cases that this might arise for? I'm not entirely sure of the context, but at what point will it make more sense to have a reject list, rather than an accept one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing this out. I looked at the other constructors of CommandName and I think DocTest and Lint probably need to do this too, so I added them. I also changed the type of cmdUseDamlYamlArgs, now it's a predicate on CommandName so we have to be exhaustive, and I've added comments explaining the Falses

(errMsgs, parseResult) = parse args
Command _ _ io <- handleParseResult parseResult
forM_ errMsgs $ \msg -> do
hPutStrLn stderr msg
withProgName "damlc" io

-- | Commands for which we add the args from daml.yaml build-options.
cmdUseDamlYamlArgs :: [CommandName]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other cases that this might arise for? I'm not entirely sure of the context, but at what point will it make more sense to have a reject list, rather than an accept one

Copy link
Contributor

@samuel-williams-da samuel-williams-da left a comment

Choose a reason for hiding this comment

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

Thanks @akrmn, you've prevented future tickets ;)

@samuel-williams-da
Copy link
Contributor

As a side note, is there any damage to forwarding flags that aren't used?

@akrmn
Copy link
Contributor Author

akrmn commented Mar 10, 2023

As a side note, is there any damage to forwarding flags that aren't used?

oh, this is a fair point, you'd get an error if build-options includes an option that works for some but not all commands. Actually, this is already the case, if you have build-options: [--incremental=auto], daml build works but daml repl doesn't.

I'm thinking, perhaps it would make sense to have different fields in daml.yaml, e.g. build-options, repl-options, specific to each command, plus an options that gets added to every command. That way --incremental=auto could be under build-options while something more common like --target=1.dev could be in the shared options.

I think the problematic flags are few, though, since most commands use optionsParser (only a few flags change depending on the arguments given), so I think we can fix those later

EDIT: #16518

Copy link
Contributor

@dylant-da dylant-da left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! The fairly ad-hoc way we collect arguments/config could be a good place to target some clean-up as part of the "improved tooling" drive.

@akrmn akrmn merged commit 3ce15c6 into main Mar 14, 2023
17 checks passed
@akrmn akrmn deleted the akrmn/daml-yaml-init branch March 14, 2023 09:44
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.

daml damlc init does not pick up build-options from daml.yaml
3 participants