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

Add cadaver recipe #3266

Merged
merged 10 commits into from Nov 24, 2018
Merged

Add cadaver recipe #3266

merged 10 commits into from Nov 24, 2018

Conversation

pjht
Copy link
Contributor

@pjht pjht commented Oct 24, 2018

This is my submission for the google code-in task

Copy link
Contributor

@Begasus Begasus left a comment

Choose a reason for hiding this comment

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

Please adjust the suggested changes

net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
@Begasus
Copy link
Contributor

Begasus commented Oct 24, 2018

There is a trailing white-space, check the line endings for spaces

@pjht
Copy link
Contributor Author

pjht commented Oct 24, 2018

I've implemented the changes.

@scottmc
Copy link
Member

scottmc commented Oct 24, 2018

Seems you still have a trailing white space issue that travis-ci is complaining about.

@pjht
Copy link
Contributor Author

pjht commented Oct 24, 2018

Fixed

@pjht
Copy link
Contributor Author

pjht commented Oct 24, 2018

All good.

Copy link
Member

@fbrosson fbrosson left a comment

Choose a reason for hiding this comment

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

minor style issues

net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Show resolved Hide resolved
@Begasus
Copy link
Contributor

Begasus commented Oct 25, 2018

I'll leave the rest of the changes need to be done followed up by @fbrosson , thanks for the contribution @pjht !

@fbrosson
Copy link
Member

Thanks @Begasus! You did the most difficult part of the mentoring!
To tell the truth I did not even dare to write a review yesterday 😄

@pjht
Copy link
Contributor Author

pjht commented Oct 25, 2018

Did the changes

Copy link
Member

@fbrosson fbrosson left a comment

Choose a reason for hiding this comment

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

Oops, we have a small typo: {lib,devel}:libzlib ➡️ {lib,devel}:libz

net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
net-misc/cadaver/cadaver-0.23.3.recipe Outdated Show resolved Hide resolved
Copy link
Member

@fbrosson fbrosson left a comment

Choose a reason for hiding this comment

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

Oops, I tried cadaver on a site served through https and got this warning: SSL is not enabled.
It seems we need to pass some configure option to enable ssl support, because right now this is what we have:

objdump -x /bin/cadaver |grep NEEDED
  NEEDED      libreadline.so.7
  NEEDED      libncurses.so.6
  NEEDED      libintl.so.8
  NEEDED      libnetwork.so
  NEEDED      libexpat.so.1
  NEEDED      libroot.so

@Begasus
Copy link
Contributor

Begasus commented Oct 25, 2018

Think I already mentioned openssl yesterday on IRC :)
EDIT, nvm it's in the requirements, maybe use an EXPORT LIBS = for -lnetwork (or patch configure to check for that)?

@pjht
Copy link
Contributor Author

pjht commented Oct 25, 2018

libzlib changed to libz

@Begasus
Copy link
Contributor

Begasus commented Oct 25, 2018

@fbrosson for me it's been a good task, so for creating the recipe it's ok for me, the part failing with ssl could be dealt with later

@Begasus
Copy link
Contributor

Begasus commented Oct 25, 2018

hmm ... did you check if config.log is looking for libcrypto also? ;)

@pjht
Copy link
Contributor Author

pjht commented Oct 25, 2018

So are you going to mark my task complete?

@Begasus
Copy link
Contributor

Begasus commented Oct 25, 2018

I can't see your task in my instances, so I guess @fbrosson needs to approve then :)

@fbrosson
Copy link
Member

I'm adding a "help-wanted" label in case someone has an idea on how to fix the ssl issue.
@pulkomandy?

@fbrosson
Copy link
Member

I can't see your task in my instances, so I guess @fbrosson needs to approve then :)

Done :)
That said, if I'm not mistaken, one does not need to be mentor on a task to approve it. Any mentor on the Org can approve any task from that same Org.

@kenmays
Copy link
Contributor

kenmays commented Oct 25, 2018

@pjht - Add neon to your build requirements. Use --with-neon flag in configure.

@pjht
Copy link
Contributor Author

pjht commented Oct 25, 2018

@kenmays I added neon and got this error:
checking for neon-config... /bin/neon-config
checking linking against neon... yes
configure: incompatible neon library version 0.30.2: wanted 0.27 28 29
configure: error: could not use external neon library

@pjht
Copy link
Contributor Author

pjht commented Oct 25, 2018

@kenmays I did

1 similar comment
@pjht
Copy link
Contributor Author

pjht commented Oct 25, 2018

@kenmays I did

@pjht pjht closed this Oct 25, 2018
@pjht pjht reopened this Oct 25, 2018
@scottmc
Copy link
Member

scottmc commented Oct 25, 2018

seems cadaver is dead. There i said it. The joke was right in front of us. Anyways, there's likely a patch floating around that adds support for newer neon, check debian or others. Lets mark this task complete and leave this open until the neon/ssl issue is figured out.

@scottmc
Copy link
Member

scottmc commented Oct 25, 2018

there's likely a patch floating around that adds support for newer neon, check debian or others. Lets mark this task complete and leave this open until the neon/ssl issue is figured out.

likely some hints here on how to fix this:
https://gitweb.gentoo.org/repo/gentoo.git/tree/net-misc/cadaver/cadaver-0.23.3.ebuild
perhaps wrap this recipe up as is, or mark as broken and merge, then add a new issue to resolve the neon/ssl issue, which might take a bit more work.

@kenmays
Copy link
Contributor

kenmays commented Oct 25, 2018

@pjht - You'll need to patch the check neon version in your configure.ac file to allow it to use neon 0.30.

@scottmc
Copy link
Member

scottmc commented Oct 25, 2018

@pjht - You'll need to patch the check neon version in your configure.ac file to allow it to use neon 0.30.

It's likely a rabbit hole... you'll need to patch configure.ac to use neon 0.30, then you'll need to add autoreconf, then you'll find that it needs a newer neon.m4 file added, then ... it'll likely be several more iterations until you arrive at the final solution. I say mark it at broken on all architectures, get it merged, and open a new ticket to fix the neon issue, and it's likely to take a few more steps to resolve.

@fbrosson
Copy link
Member

We don't need to mark the recipe as broken because it is working. It's just that it will only be able to access http sites.
That said, using a WebDAV service on http is not a good idea unless one is not afraid of sending passwords through http.

In other words, if we merge this PR then I think it would be OK to keep it enabled.
Hmm, why don't we just mark it as untested?

Anyway, there is no urgency, so we can keep the PR open for now and think about it for the next days.

@scottmc
Copy link
Member

scottmc commented Oct 25, 2018

It's my understanding that if it is marked as working then it could end up in HaikuDepot? I'd like to prevent it from getting there until the SSL issue is fixed. Would marking it as untested have the same effect?

@fbrosson
Copy link
Member

fbrosson commented Oct 26, 2018

Yep, marking it as untested will make the build bots skip it, so pre-built packages won't be available. I think this can be a nice scenario for now if we don't find a better fix.
This would be nice because any user willing to try this ssl-disabled cadaver can still build it from sources without having to edit the recipe locally.

I have cadaver on Debian 9 (Stretch) and am able to access (read&write) https webdavs.

$ cadaver --version
cadaver 0.23.3
neon 0.30.2: Library build, IPv6, libxml 2.9.4, zlib 1.2.8, GNU TLS 3.5.6.
readline 7.0

$ cadaver https://example.org/dav/
Authentication required for private on server `example.org':
Username: me
Password: 
dav:/dav/> ls
Listing collection `/dav/': succeeded.

(The webdav site I used is not example.org but a private remote webdav I control.)

I haven't investigated yet all possible paths (i.e. autoreconf, looking for a patch from a Linux distro such as Debian, Gentoo or ArchLinux) so we might be able to fix the ssl issue before the merge.
I'd say that if we don't find a fix before next week then maybe we can mark cadaver as untested and still merge.

@Begasus
Copy link
Contributor

Begasus commented Oct 29, 2018

It is possible it seems :) (from config.log)

configure:9610: using neon library 0.30.2
configure:9657: SSL is supported by neon

@Begasus
Copy link
Contributor

Begasus commented Oct 29, 2018

Ok, got it build with the 0.30.2 version from neon, not sure how to test though :) (with a small patch in configure)
EDIT, build for x86_gcc2 and x86_64
SECOND EDIT :) output from objdumb on x86_64

~> objdump -x /bin/cadaver |grep NEEDED
    NEEDED               libreadline.so.7
    NEEDED               libncurses.so.6
    NEEDED               libintl.so.8
    NEEDED               libneon.so.27
    NEEDED               libnetwork.so
    NEEDED               libz.so.1
    NEEDED               libssl.so.1.0.0
    NEEDED               libcrypto.so.1.0.0
    NEEDED               libxml2.so.2
    NEEDED               libroot.so

@Begasus
Copy link
Contributor

Begasus commented Oct 29, 2018

@pjht - Add neon to your build requirements. Use --with-neon flag in configure.

When neon is provided in the REQUIRES and REQUIRES_devel the --with-neon flag isn't required for configure (provided configure is patched)

This is an update from the master repo. Will be merged instantly
@Begasus
Copy link
Contributor

Begasus commented Nov 3, 2018

This could be used as a pointer on how to patch configure :)
https://github.com/Homebrew/homebrew-core/blob/master/Formula/cadaver.rb

Also use "make install" instead of "make install-strip" because
there is no install-strip target.
@fbrosson fbrosson merged commit cd57008 into haikuports:master Nov 24, 2018
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.

None yet

5 participants