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

Creating files #142

Closed
wants to merge 1 commit into from
Closed

Creating files #142

wants to merge 1 commit into from

Conversation

domenkozar
Copy link
Member

@domenkozar domenkozar commented Nov 29, 2022

The idea is that you can create files like:

{
  files."devcontainer.json".json = { name = "myenv"; };
  files.".editorconfig".ini = { "*" = { end_of_line = "lf"; }; };
  files."README.md".text = "Here we go!";
}

And running devenv shell would print:

$ devenv shell
...
Creating devcontainer.json
Creating README.md
Creating .editorconfig

TODO

  • write documentation introduction
  • implement different format generation
  • assert that only one format can be used per file
  • keep the format list open for extension

@domenkozar domenkozar changed the title WIP: Creating files Creating files Nov 29, 2022
@hugosenari
Copy link
Contributor

Questions:

implement different format generation

Would better make sure our design is extensible since the number of output formats isn't defined?

assert that only one format can be used per file

If we resolve all other types to files."fileName.ext".text it would automatically reject config if two files point to same the path?

On other side, should we change type of text to be lines (that auto merge)?

@domenkozar
Copy link
Member Author

Questions:

implement different format generation

Would better make sure our design is extensible since the number of output formats isn't defined?

That's a good point, how can we make the design extensible without sacrificing the simplicity of the user api?

assert that only one format can be used per file

If we resolve all other types to files."fileName.ext".text it would automatically reject config if two files point to same the path?

What would reject the config of two files in such case?

On other side, should we change type of text to be lines (that auto merge)?

For some files you would want to append, for some you would want to override. Not sure.

@hugosenari
Copy link
Contributor

hugosenari commented Nov 29, 2022

That's a good point, how can we make the design extensible without sacrificing the simplicity of the user api?

🤷

What would reject the config of two files in such case?
Ie:

{
  files."devcontainer.json".json = { name = "myenv"; };
  files."devcontainer.json".ini  = { "*" = { end_of_line = "lf"; }; };  
}

If both resolve to files."devcontainer.json".text (in my code is file."devcontainer.json".text because of infinity recursion), eval modules will complain that value was settled twice, unless we use mkForce (mkDefault, etc..) or type is lines (in this case they will be appended to the end)

@hugosenari
Copy link
Contributor

That's a good point, how can we make the design extensible without sacrificing the simplicity of the user api?

@bew once told me that we could add only text option and user does whatever to convert into the text.

I don't agree, but is good seen things through a different lens

@bobvanderlinden
Copy link
Contributor

Nice! This can really come in handy!

I'm interested whether this should be inside enterShell or whether some kind of setup script should be introduced. Mostly because these files are 'conflict-prone'. What should happen upon conflicts? Should it error? Should it warn? Should it silently overwrite?

If erroring/warning, should it not load the shell? How can you make sure it reattempts to apply the files?

An approach I took on my devenv config is to have a setup script that you usually run once. It includes some authentication assertions (.npmrc), copy some (project specific) config.example.json to config.json file and run npm ci. Doing this each enterShell is too much, which is why I placed it in a script. If anything fails, you attempt to fix it and rerun the script again. Not ideal, but it served my purpose for now. Might be something to consider here as well?

@hugosenari
Copy link
Contributor

I'm interested whether this should be inside enterShell or whether some kind of setup script should be introduced. Mostly because these files are 'conflict-prone'. What should happen upon conflicts? Should it error? Should it warn? Should it silently overwrite?

I'm favoring of overwriting, as we need to set a single source of truth. Ideally, files should add header banner 'This files was created by X Y Z do not edit it', but isn't always viable (JSON, no comments),

Other problem would be updates, we can't diff existing files with previous generation to check if changes are updates or someone edited it directly. Any change would'be a conflict.

But we should not add any option that creates files by default, only when enabled. ie: Devenv shouldn't control gitignore file, unless someone configure it to do. #178

An approach I took on my devenv config is to have a setup script that you usually run once. It includes some authentication assertions (.npmrc), copy some (project specific) config.example.json to config.json file and run npm ci. Doing this each enterShell is too much, which is why I placed it in a script. If anything fails, you attempt to fix it and rerun the script again. Not ideal, but it served my purpose for now. Might be something to consider here as well?

The only problem of this is to have files out of sync more often, ie:

  • edit a nix file stage.nix (cfg for stage environment)
  • activate the devenv
  • run setup
  • create a zip file for stage deployment.

In steps above, user forgot to run setup, deploying stage with old conf file.
He can forget to activate either, but we minimized the problem.
With direnv the problem almost disappear.

@domenkozar
Copy link
Member Author

Nice! This can really come in handy!

I'm interested whether this should be inside enterShell or whether some kind of setup script should be introduced. Mostly because these files are 'conflict-prone'. What should happen upon conflicts? Should it error? Should it warn? Should it silently overwrite?

If erroring/warning, should it not load the shell? How can you make sure it reattempts to apply the files?

An approach I took on my devenv config is to have a setup script that you usually run once. It includes some authentication assertions (.npmrc), copy some (project specific) config.example.json to config.json file and run npm ci. Doing this each enterShell is too much, which is why I placed it in a script. If anything fails, you attempt to fix it and rerun the script again. Not ideal, but it served my purpose for now. Might be something to consider here as well?

My thinking is that conflicts should be resolved at Nix level, if you define two of the same files. Otherwise Nix assumes it's the source of truth what should be the latest version of the declared file: it will override.

I'm thinking to split enterShell into motd (which would be more about printing into the shellandsetup` that would take care of setting up the environment.

I'm not sure creating files will slow things down that much given that everyone has an SSD these days. But for slower operations we might consider some command to run things manually.


let
types = lib.types;
ini = pkgs.formats.ini { };
Copy link
Contributor

Choose a reason for hiding this comment

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

@adrian-gierakowski
Copy link

I'm not sure creating files will slow things down that much given that everyone has an SSD these days. But for slower operations we might consider some command to run things manually.

I'd say there should be an option to disable auto-creation and provide a command\script which could be used to recreate the files manually. A script which checks that all files are in expected state would also be useful for running checks in CI.

@adrian-gierakowski
Copy link

I'm not sure creating files will slow things down that much given that everyone has an SSD these days. But for slower operations we might consider some command to run things manually.

I'd say there should be an option to disable auto-creation and provide a command\script which could be used to recreate the files manually. A script which checks that all files are in expected state would also be useful for running checks in CI.

An idea of how disabling of auto-creation could be implemented:
#209 (comment)

For manual execution we could add create-all option of package type (like this one), and then have it added to the devenv packages

@adrian-gierakowski
Copy link

@hugosenari

On other side, should we change type of text to be lines (that auto merge)?

Personally I think the default should be not to merge as auto-merging could lead to surprising bugs. But we could let users choose with options like (pseudocode):

{
  options.merge.enable = { type = bool; default = false; };
  options.merge.strategy = { type = optionType; default = lines; };
}

And then set text.type = config.merge.strategy. Apart form lines, one might want to use commas or envVars, or some custom strategy.

@adrian-gierakowski
Copy link

@domenkozar as mentioned on the bottom of this post I believe it would be beneficial to have this implemented in a "platform" agnostic way, so that anyone can re-use this regardless if they use devenv, devshell, or whatever else. The standalone module should contain option definitions and logic for creating self contained scripts which could be executed from anywhere to create files from given root dir. The only thing to be done on devenv side would be to import the module and add devenv specific config.

I'm happy to work on this if we can agree on an interface.

@domenkozar
Copy link
Member Author

text.type = config.merge.strategy

I don't think it's possible for options to depend on the config - that creates an infinity error.

I believe it would be beneficial to have this implemented in a "platform" agnostic way

I agree, if we can unite the design goals :)

@adrian-gierakowski
Copy link

text.type = config.merge.strategy

I don't think it's possible for options to depend on the config - that creates an infinity error.

I used to think that but it seems to work here, and here.

@adrian-gierakowski
Copy link

I believe it would be beneficial to have this implemented in a "platform" agnostic way

I agree, if we can unite the design goals :)

Sure!

@hugosenari do you have any further comments apart from the feedback you’ve provided here so far?

@domenkozar
Copy link
Member Author

if someone wants to pick this up that would be great!

@bobvanderlinden
Copy link
Contributor

bobvanderlinden commented Nov 14, 2024

Just today I ran into a use-case for this. I'll have a look cherry-picking this PR and bringing it up-to-date 👍

@bobvanderlinden bobvanderlinden mentioned this pull request Nov 14, 2024
@bobvanderlinden
Copy link
Contributor

I've created #1590 against the latest main. Mostly in line with this PR. Feedback is welcome.

@domenkozar domenkozar closed this Nov 14, 2024
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