Skip to content

Makefile improvements #32

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

Merged
merged 2 commits into from
Mar 31, 2019
Merged

Conversation

maximbaz
Copy link
Member

Addressing feedback in #31

@maximbaz maximbaz mentioned this pull request Mar 30, 2019
6 tasks
Copy link
Contributor

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

LGTM, if there are any other problems during packaging when I get to it, I'll let you know. Thanks for being considerate <3

@maximbaz maximbaz merged commit 2925002 into browserpass:master Mar 31, 2019
@infinisil
Copy link
Contributor

Trying to set BIN to something different I found something that needs changing in the Makefile:

browserpass: *.go **/*.go
go build -ldflags $(GO_LDFLAGS) -gcflags $(GO_GCFLAGS) -asmflags $(GO_ASMFLAGS) -o $@

The -o $@ always makes the binary name be browserpass, should be changed to $(BIN) (also in the build commands following that one)

@maximbaz
Copy link
Member Author

Those commands intentionally produce binaries with different names, because I need all of them to make a Github release :)

I thought for BIN as purely a way to configure install target, i.e. if you downloaded linux release from Github, the binary would be called browserpass-linux64, and thus to install you would run make BIN=browserpass-linux64 install.

Maybe simply do make browserpass followed by mv browserpass <newName>, and then make BIN=<newName> install, would that work for you?

@infinisil
Copy link
Contributor

Hmm I see, was trying to avoid having to manually rename the binary, but yeah that works.

@maximbaz
Copy link
Member Author

It's temporary, I guess a month from now you'll be able to cleanup the build commands and return to browserpass as a binary name 😉

@infinisil
Copy link
Contributor

Almost done with packaging this for Nix/NixOS. Two suggestion I have regarding the linking targets in the Makefile:

  • Using these currently results in no output. Adding -v to ln would make it output what is being linked
  • If the path to be linked isn't valid, you end up with an invalid symlink without the user noticing. I suggest adding an error message in that case.

@infinisil
Copy link
Contributor

Oh and I just saw that you're using the host files where the policies files are supposed to be:

ln -sf "$(LIB_DIR)/browserpass/hosts/chromium/$(APP_ID).json" "/etc/chromium/policies/managed/" \

@maximbaz maximbaz deleted the makefile-improvements branch April 1, 2019 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants