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

Remove generated files from source control #4191

Closed
1 of 2 tasks
osmaczko opened this issue Oct 24, 2023 · 10 comments · Fixed by #5878
Closed
1 of 2 tasks

Remove generated files from source control #4191

osmaczko opened this issue Oct 24, 2023 · 10 comments · Fixed by #5878

Comments

@osmaczko
Copy link
Contributor

osmaczko commented Oct 24, 2023

Problem

Remove the output of make generate from source control. Nowadays, almost every PR produces a ridiculous number of changes because contributors use different versions of the tools responsible for generation. This makes PRs difficult to review since most of the changes are unrelated to the actual issue.

Acceptance Criteria

  • remove all generated files from source control
  • enforce the use of the Nix shell in the Makefile to ensure uniform tool usage
@osmaczko
Copy link
Contributor Author

osmaczko commented Oct 24, 2023

@jakubgs I would love to hear your opinion. Is there anything else I should think about?

@osmaczko osmaczko self-assigned this Oct 24, 2023
@jakubgs
Copy link
Member

jakubgs commented Oct 24, 2023

I'm all for not storing generated files in the repo, that is silly. We can always do that at build time.

But the fundamental issue is that we're using varied version of tooling, and this could be easily remedied by using an already defined Nix shell on dev environments:
https://github.com/status-im/status-go/blob/develop/shell.nix

We could achieve this the same way we do it in status-mobile, by enforcing use of the Nix shell in the Makefile, like so:

# WARNING: This has to be located right before all the targets.
SHELL := ./nix/scripts/shell.sh

shell: export TARGET ?= default
shell: ##@prepare Enter into a pre-configured shell
ifndef IN_NIX_SHELL
	@ENTER_NIX_SHELL
else
	@echo "${YELLOW}Nix shell is already active$(RESET)"
endif

https://github.com/status-im/status-mobile/blob/ca2b3abb1c3091712fc2d5e4300185394a6fb77f/Makefile#L67-L76

Of course some shells would not use Nix shell, but that can be always overriden using the SHELL variable, for example:

_fix-node-perms: SHELL := /bin/sh
_fix-node-perms: ##@prepare Fix permissions so that directory can be cleaned
	$(shell test -d node_modules && chmod -R 744 node_modules)
	$(shell test -d node_modules.tmp && chmod -R 744 node_modules.tmp)

https://github.com/status-im/status-mobile/blob/ca2b3abb1c3091712fc2d5e4300185394a6fb77f/Makefile#L123-L126

It would be fairly simple to adopt Nix setup from status-mobile to status-go, me and @yakimant can help.

@osmaczko
Copy link
Contributor Author

Thanks for the quick response. That makes a lot of sense and will keep the CI scripts untouched. I would apply both solutions: remove generated files from the repo and enforce using the Nix shell in the Makefile. Shall I create a separate task for adopting the Nix setup from status-mobile to status-go, and should I assign it to you or @yakimant?

@jakubgs
Copy link
Member

jakubgs commented Oct 24, 2023

A separate issue might make sense. It would be fairly easy for me or Anton to do it, but how about you do it and learn a bit about our Nix setup in the process? I'm happy to explain how this should be done and help debug potential issues.

@osmaczko
Copy link
Contributor Author

I'm happy to learn. Maybe I'll have time to pick it up next week. I'll ask for explanations then. Thanks!

@osmaczko
Copy link
Contributor Author

@jakubgs unfortunately, I don't think I will have time for this task anytime soon, as I am already full of higher-priority, protocol-level tasks.

15k+ lines of changes on almost each status-go PR is killing me. Would you be a lifesaver and handle the Nix setup for status-go, please 🙏 ? Also, let me know if you'd like me to create a separate task for this mission.

@jakubgs
Copy link
Member

jakubgs commented Nov 17, 2023

That's fine @osmaczko, understandable.

I don't think this is easily doable right now without making devs lives a bit worse in another way, see this issue:

Unless developers agree that updating the vendor SHA is less annoying than dealing with that massive vendor folder.

I'm thinking @apentori could take this task, as he wants to work more on CI and Nix things.

@jakubgs
Copy link
Member

jakubgs commented Nov 17, 2023

If we don't use flakes, it should be in theory doable to script updating the SHA(have not tested this). And that in turn could possibly be added as a Git hook, to avoid devs having to remember to make the change themselves.

@jakubgs jakubgs assigned apentori and unassigned osmaczko Nov 18, 2023
@osmaczko
Copy link
Contributor Author

That's fine @osmaczko, understandable.

Thank you.

Unless developers agree that updating the vendor SHA is less annoying than dealing with that massive vendor folder.

@jakubgs correct me if I am wrong, but is it necessary to get rid of the vendor folder in order to make nix-shell work?

Updating SHA manually still sounds better than varied dev environments. @cammellos may I ask for your opinion here?

If we don't use flakes, it should be in theory doable to script updating the SHA(have not tested this). And that in turn could possibly be added as a Git hook, to avoid devs having to remember to make the change themselves.

That would be great.

@jakubgs
Copy link
Member

jakubgs commented Nov 22, 2023

My recommendation would be to do this in at least 3 steps:

  1. Introduce Nix setup from status-mobile and use it only in CI first, minimal or no Makefile changes.
  2. Change the Makefile to use the shell implicitly and adjust targets that don't need a Nix shell, remove obsolete ones.
  3. Remove generated files, add the to .gitignore, and make sure they are generated for all types of builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants