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

Suggestion to add devcontainer configuration #442

Closed
hsloot opened this issue May 2, 2023 · 8 comments
Closed

Suggestion to add devcontainer configuration #442

hsloot opened this issue May 2, 2023 · 8 comments
Labels
contribution Thank you to the contributor! docker implemented implemented tag means that either an enhancement or feature request has been implemented, or bugfix

Comments

@hsloot
Copy link
Contributor

hsloot commented May 2, 2023

Summary

The project might benefit from providing Github's Codespaces or vscode's devcontainers configurations to allow contributors to use a Docker-based dev environment. I have a branch with a draft version, but it would require some polishing and considerations; see https://github.com/hsloot/latexindent.pl/tree/add-devcontainer. In my view, the main benefit would be that it makes it easy for inexperienced contributors (latex users but are inexperienced with Perl) to test small changes before creating PRs or debug their code when PR checks fail.

Considerations

  • Align the devcontainer image with the already existing Docker image created for using latexindent?
  • Align with the test workflow for pull requests such that the test workflow passes if the tests run successfully in a new devcontainer instance?

TODOs

  • Provide documentation.
  • Improve documentation about building and testing.
  • Adjust Dockerfile to already install all required Perl modules.
@cmhughes
Copy link
Owner

cmhughes commented May 2, 2023

Many thanks, this sounds good, please feel encouraged to take it on.

@eggplants any input/advice?

@cmhughes cmhughes added contribution Thank you to the contributor! docker labels May 2, 2023
@hsloot
Copy link
Contributor Author

hsloot commented May 2, 2023

I created the draft PR #443; consider trying it (e.g., in Codespaces or locally with vscode) to provide some feedback and whether all usual features for development are present.

Some comments:

  • I extended the README's testing section as it requires copying some files to /usr/local/bin (I used the test workflow as a template).
  • I also added a note about using the devcontainer to the README.
  • The cruft directory tests fail since ~/Desktop/tmp does not exist in this devcontainer. I used $(mktemp -d) instead.
  • The Dockerfile ./.devcontainer/Dockerfile is based on ./Dockerfilewith three changes: I reduced it to the base image and the apt-get command, I added locale configuration, and I installed the required Perl modules similar to the helper script.

@cmhughes
Copy link
Owner

cmhughes commented May 3, 2023 via email

@hsloot
Copy link
Contributor Author

hsloot commented May 3, 2023

There are three options to use this:

The point-of-entry should be my feature branch https://github.com/hsloot/latexindent.pl/tree/add-devcontainer (meaning you start the Codespace from this branch on my fork or the vscode development container by checking out this branch); see screenshot below.

If you have not used vscode: it has an integrated terminal that you may use to test all your usual development workflows.

Greenshot 2023-05-03 08 45 08

@hsloot
Copy link
Contributor Author

hsloot commented May 3, 2023

I hope this helps. It would be great if you could try some of your usual development workflows (testing, local building, documentation, ...) to see if the Dockerfile must be extended such that the image contains all the required features. There is also the option to include further vscode extensions, but that is probably not necessary here ...

@cmhughes
Copy link
Owner

Thanks for this, I've finally been able to look at it, apologies for the delay.

what I've done to test it

  1. as you recommended, I started from your branch at https://github.com/hsloot/latexindent.pl/tree/add-devcontainer
  2. I used your screenshot as a guide, and clicked Create codespace on add-devcontainer which opened up a codespace (my first ever experience of this!)
  3. I ran the following commands
chmod +x latexindent.pl
cp latexindent.pl /usr/local/bin
cp -r LatexIndent /usr/local/bin
cp defaultSettings.yaml /usr/local/bin

as follows:

image

  1. I then navigated to the test-cases directory and ran test-cases.sh:

image

  1. I then made a change to latexindent.pl by clicking on Explorer in the left-hand pane, and ran ./latexindent.pl from the terminal:

image

  1. Running git diff then details the changes:

image

  1. From here, presumably I can commit my changes into my branch...?

my feedback

I think this looks great, many thanks! I think this can be merged, and doesn't needed any more documentation than what you have done (thanks!), because (if I've understood the intention of this) this will be for a specific set of developers and it's not for users.

So, it looks ready to me, but what do you think?

@hsloot
Copy link
Contributor Author

hsloot commented May 21, 2023

Great! So, I think one should be able to make commits and push when the codespace is started from a personal fork or if one needs write permissions to the directory it is linked to; see https://docs.github.com/en/codespaces/developing-in-codespaces/using-source-control-in-your-codespace#about-automatic-forking.

I also think you can merge it. However, you should keep in mind two hidden dependencies:

  • I created the codespaces-Dockerfile using your Dockerfile for the docker-version of latexindent. You might want to keep those aligned if either is changed in the future. I think it would be possible to create a base Dockerfile and "stack" both of the currently existing ones on top of that.
  • The Dockerfile installs the Perl dependencies manually. Hence, Dockerfile and helper scripts might also diverge if not manually aligned.

However, I do think these are minor issues (which, of course, could be fixed if you notice that something is broken). Also, both are probably not that difficult to do, but since I am not that familiar with Docker myself, I am not too sure about how to do it.

I will change the status so that you can merge it.

cmhughes added a commit that referenced this issue May 21, 2023
@cmhughes cmhughes added the implemented implemented tag means that either an enhancement or feature request has been implemented, or bugfix label May 21, 2023
@cmhughes
Copy link
Owner

Great, many thanks! This is implemented as of 6288f5b

Thanks so much :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution Thank you to the contributor! docker implemented implemented tag means that either an enhancement or feature request has been implemented, or bugfix
Projects
None yet
Development

No branches or pull requests

2 participants