-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
gophish: add new package #23052
base: master
Are you sure you want to change the base?
gophish: add new package #23052
Conversation
1fd153b
to
2d98e2f
Compare
Running |
mail/gophish/files/gophish.init
Outdated
procd_set_param stdout 1 | ||
procd_set_param stderr 1 | ||
procd_set_param user gophish | ||
procd_set_param env GOPHISH_VERSION="0.12.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I dont think this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the environment variable? The application requires this. Normally it is set by a file named VERSION in the application's working directory. I patched Gophish to support an environment variable instead to better suit procd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @jefferyto about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense to read the version from an environment variable, because the version number is a constant and not a configuration value that needs to be changed. (The version number also needs to be hardcoded into the init script.)
I suggest patching the full path of the VERSION file into the program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I simply patch the version directly in the application now.
3b75b12
to
d2202c0
Compare
@jefferyto, would you mind taking a look at this? I think @BKPepe wanted your opinion on my use of the environment variable in gophish.init. He has some comments above. |
mail/gophish/Makefile
Outdated
PKG_BUILD_FLAGS:=no-mips16 | ||
|
||
GO_PKG:=github.com/gophish/gophish | ||
GO_PKG_LDFLAGS_X:=$(GO_PKG)/cmd.Version=$(PKG_VERSION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setting this version value necessary / used in the program somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the GO_PKG_LDFLAGS definition.
dialect: sqlite3 | ||
- import: github.com/mattn/go-sqlite3 | ||
\ No newline at end of file | ||
+ import: modernc.org/sqlite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using modernc.org/sqlite instead of github.com/mattn/go-sqlite3 strictly necessary? This feels like a change that should be accepted upstream first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I proposed this upstream last year:
The sqlite upstream selected uses cgo, which make cross-compiling more difficult. I replaced this with a pure-Go implementation.
Signed-off-by: W. Michael Petullo <[email protected]>
Maintainer: me
Compile tested: x86_64 master
Run tested: x86_64 master