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

openfortivpn 1.4.0 (new formula) #14344

Closed
wants to merge 1 commit into from
Closed

openfortivpn 1.4.0 (new formula) #14344

wants to merge 1 commit into from

Conversation

mnencia
Copy link
Contributor

@mnencia mnencia commented Jun 7, 2017

openfortivpn is a client for PPP+SSL VPN tunnel services. It spawns a pppd
process and operates the communication between the gateway and this process.

It is compatible with Fortinet VPNs.

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@ilovezfs
Copy link
Contributor

ilovezfs commented Jun 8, 2017

@BrewTestBot test this please

@ilovezfs ilovezfs added the new formula PR adds a new formula to Homebrew/homebrew-core label Jun 8, 2017
system "aclocal"
system "autoconf"
system "automake", "--add-missing"
ENV.prepend "LDFLAGS", "-L#{Formula["openssl"].opt_lib}"
Copy link
Contributor

Choose a reason for hiding this comment

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

These paths can't be specified as arguments to configure?

Copy link
Contributor Author

@mnencia mnencia Jun 10, 2017

Choose a reason for hiding this comment

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

I've checked, and unfortunately this is the only way with the current source.

This is basically a transcription of the macOS installing instructions provided by the software author in https://github.com/adrienverge/openfortivpn/blob/master/README.md

depends_on "openssl"

def install
system "aclocal"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no autogen.sh script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there isn't

system "make", "install"
end
test do
system bin/"openfortivpn", "--version"
Copy link
Contributor

Choose a reason for hiding this comment

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

A test that checks more functionality would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a VPN software, I've no idea on how to check its functionality without connecting to a server. --help and --version are the only informative functions provided by openfortivpn

The only tests included in the source are about code formatting, so nothing useful to check if the software works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, thanks.

@dunn
Copy link
Contributor

dunn commented Jun 10, 2017

Would you mind asking the developers if they'd consider adding a autogen.sh-style script, and patching configure to allow specifying the OpenSSL path?

@mnencia
Copy link
Contributor Author

mnencia commented Jun 12, 2017

I've made a pull request to openfortivpn to fix these issues. adrienverge/openfortivpn#144

@JCount
Copy link
Contributor

JCount commented Jun 25, 2017

@mnencia If you would like, it may make the most sense to close this PR and open a new one for the new 1.4.0 release, but if you want to simply update this in place that is fine as well. Good work with the openfortivpn PR. 😄

openfortivpn is a client for PPP+SSL VPN tunnel services. It spawns a pppd
process and operates the communication between the gateway and this process.

It is compatible with Fortinet VPNs.
@mnencia mnencia changed the title openfortivpn 1.3.1 (new formula) openfortivpn 1.4.0 (new formula) Jun 26, 2017
@mnencia
Copy link
Contributor Author

mnencia commented Jun 26, 2017

I've updated this pull request to 1.4.0 version

@dunn
Copy link
Contributor

dunn commented Jun 26, 2017

Thanks for contributing to Homebrew! Merged as 22d6d1d.

@DimitriPapadopoulos
Copy link
Contributor

DimitriPapadopoulos commented Jul 27, 2017

@mnencia @dunn FYI the autotools files in openfortivpn have been changed again. They now make use of pkg-config which seems like the right thing to do on Linux and BSD systems. To pick an alternate OpenSSL library, you may have to define PKG_CONFIG_PATH. Does this work with Homebrew or do you need option --with-openssl back?

See adrienverge/openfortivpn@70c5ed1.

@mnencia
Copy link
Contributor Author

mnencia commented Jul 27, 2017

It should work.
There are two other formula that are using that approach: curl.rb and xmount.rb
However I'd like to know what @dunn thinks from the style point of view.

@mnencia mnencia deleted the openfortivpn branch July 27, 2017 09:03
@mrbaseman
Copy link

My colleague has reported that the new formula works like charm 👍
To my understanding it uses the 1.4.0 release whereas the master branch has switched to pkg-config. On the other hand I did not see the formula itself making use of either of these options. But probably the core brings in support for such configure options (maybe both variants?).

@mnencia
Copy link
Contributor Author

mnencia commented Jul 31, 2017

@mrbaseman the change has not yet been released. What @DimitriPapadopoulos was asking is if this change is compatible with Homebrew or they need to modify it before the next release.

My answer is: The formula has to be updated, but the change is fine.

I don't know if @dunn wants to add anything.

@DimitriPapadopoulos
Copy link
Contributor

@mnencia Have you had the chance to test openfortivpn 1.5.0 yet?

Please note that Autotools support for OpenSSL has been rewritten in openfortivpn and hopefully standardized. The --with-openssl option is gone, you may use PKG_CONFIG_PATH instead.

@mnencia
Copy link
Contributor Author

mnencia commented Sep 1, 2017

I've tested it briefly and everything looks ok.

@DimitriPapadopoulos
Copy link
Contributor

Ooops... I just noticed recent commit bf53714 by ilovezfs.
Thank you for the feedback!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants