lapsang-boys / pippi

A modular, extensible and collaborative reverse engineering ecosystem
https://pippi.re
BSD 2-Clause "Simplified" License
7 stars 1 forks source link

Remove auto-generated code from source tree? #2

Closed karlek closed 5 years ago

karlek commented 5 years ago

As a general rule, generated files do not belong in the source code repository. [...] A few reasons for deviating from the general rule are

  • After generating them, the files are further modified by hand. In that case, they are not really generated files any more and thus belong in the VCS.

[1]

I generally agree with this view. However, as also mentioned we have to fix the bug from the golang grpc generator. But, when that bug is fixed I think it would be nice to remove the generated source from the tree. Any opinions?

[1]: https://softwareengineering.stackexchange.com/a/192115

mewmew commented 5 years ago

:+1:

mewmew commented 5 years ago

Oh, second consideration. Having generated code commited makes it Go-getable. So we should consider pros and cons also with this in mind.

karlek commented 5 years ago

If it's going to be a polyglot project, it's not going to be go-gettable for long

mewmew commented 5 years ago

It should perhaps be go-getable, crate-able (what's the Rustonic term?), etc :)

Haha, just realized, just for the sake of it, the Pippi project should use Python just to be pip-installable.

karlek commented 5 years ago

Hahah yeah, sounds better than pipp-able

karlek commented 5 years ago

Can we find the issue of the gRPC code generation bug so we can track it here? Or the status of the next library version you mentioned?

mewmew commented 5 years ago

Haha, bra timing.

Hittade precis https://github.com/golang/protobuf#packages-and-input-paths

karlek commented 5 years ago

Håller på att fixa protoc i docker-imagen så vi klarar testerna igen :+1:

mewmew commented 5 years ago

Hmm, seems like the fix didn't quite work.

u@x1 ~/D/p/proto> tree
.
├── bin
│   ├── github.com
│   │   └── lapsang-boys
│   │       └── pippi
│   │           └── proto
│   │               └── bin
│   │                   └── bin.pb.go
│   └── __pycache__
│       ├── bin_pb2.cpython-37.pyc
│       └── bin_pb2_grpc.cpython-37.pyc
mewmew commented 5 years ago

We get a github.com/lapsang-boys/pippi/proto/bin/github.com/lapsang-boys/pippi/proto/bin/bin.pg.go file generated, so the import path is duplicated.

karlek commented 5 years ago

:thinking:

mewmew commented 5 years ago

Fanns en annan option source_relative kan kika om den har med saken att göra.

mewmew commented 5 years ago

Hoppas det här inte är ett issue med Go modules (och att det kräver GOPATH).

mewmew commented 5 years ago

We should be able to use https://github.com/golang/protobuf/blob/1680a479a2cfb3fa22b972af7e36d0a0fde47bf8/proto/proto3_proto/proto3.proto#L35 as a reference, they got it working.

mewmew commented 5 years ago

ref: https://github.com/golang/protobuf/blob/1680a479a2cfb3fa22b972af7e36d0a0fde47bf8/regenerate.sh#L31

     protoc -I$dir --go_out=plugins=grpc,paths=source_relative:$dir $p
karlek commented 5 years ago

It works!

karlek commented 5 years ago

Ref: https://github.com/golang/protobuf/issues/748

mewmew commented 5 years ago

More autogenerated files left to purge.

This was discovered since https://github.com/lapsang-boys/pippi/pull/4 had more line additions than deletions. A simple sed replace should not have that effect.

2019-09-25-230334_244x73_scrot

Edit: lets keep this issue open until we know that all files in the repo are manually created.

karlek commented 5 years ago

Found another one!

$ find src/ -type f | grep -i -P "\.spec\."
src/app/loader.service.spec.ts
src/app/disassembly/disassembly.component.spec.ts
src/app/loader/loader.component.spec.ts
src/app/binary.service.spec.ts
src/app/control-bar/control-bar.component.spec.ts
src/app/hexdump/hexdump.component.spec.ts
src/app/app.component.spec.ts
src/app/disassembly.service.spec.ts
src/app/select-id/select-id.component.spec.ts
src/app/id.service.spec.ts
mewmew commented 5 years ago

Shall we add a .gitignore for *.spec.tc?

karlek commented 5 years ago

Well, either that or actually start writing tests haha

mewmew commented 5 years ago

actually start writing tests haha

Hehe, both would result in a green master :p

karlek commented 5 years ago

True true :grin: