Autotools cleanup rebased by fscheiner · Pull Request #8 · gridcf/UberFTP

fscheiner · 2020-07-09T14:26:17Z

This is a first step towards fixing #2 although it does not fix any of the points there, it does make building it a lot easier.

add missing files AUTHORS, INSTALL, NEWS
rename existing copyright and Changelog to autotools expected names.
remove (ancient) output files from autotools: Makefile.in, aclocal.m4,
compile, config.h.in, configure, depcomp, install-sh, missing
remove GPT files pkg_data_src.gpt and pkg_data_src.gpt.in
remove filelist, probably also from GPT?
add ChangeLog entry for "no-longer-delete" patch
few improvements in configure.ac:
- update version, author and email
- add check for C-compiler and preprocessor. The latter is required since
  $CPP is being used.
- need to put errormsg between [] or a comma is parsed
- update the default globus locations for source and EPEL
- include both /lib and /lib64 for the LDFLAGS.
- fix AC_INIT: 2.9-Beta needs hyphen, otherwise or make dist fails
- add foreign to AM_INIT_AUTOMAKE to ease autotools
add EXTRA_DIST to Makefile.am to make sure we get all files.

commit 23-7-2020

added support for pkg-config, CPPFLAGS and LDFLAGS.
Only CFLAGS is not used.

- add missing files AUTHORS, INSTALL, NEWS - rename existing copyright and Changelog to autotools expected names. - remove (ancient) output files from autotools: Makefile.in, aclocal.m4, compile, config.h.in, configure, depcomp, install-sh, missing - remove GPT files pkg_data_src.gpt and pkg_data_src.gpt.in - remove filelist, probably also from GPT? - add ChangeLog entry for "no-longer-delete" patch - few improvements in configure.ac: - update version, author and email - add check for C-compiler and preprocessor. The latter is required since $CPP is being used. - need to put errormsg between [] or a comma is parsed - update the default globus locations for source and EPEL - include both /lib and /lib64 for the LDFLAGS. - add EXTRA_DIST to Makefile.am to make sure we get all files. - fix AC_INIT: 2.9-Beta needs hyphen, otherwise or make dist fails - add foreign to AM_INIT_AUTOMAKE to easy autotools Ideally the check for the globus include and libs should (also) be done with pkgconfig macros, but this should at least get it to build again from both source installer *and* EPEL. Also it does not yet honour CFLAGS, LDFLAGS etc.

msalle · 2020-07-10T15:45:43Z

Hi all, I've now properly ff-ed my autotools_cleanup_rebased branch (couldn't do that beforehand), such that it itself is just a ff from the master branch. I fixed one more thing in the configure.ac (adding foreign to AM_INIT_AUTOMAKE).
I think it's ready now.

configure.ac

INSTALL

Add missing support for pkg-config, and for using environment variables CPPFLAGS and LDFLAGS. We still ignore CFLAGS which is less useful.

Running autoreconf is sufficient to get all the autotools to work.

msalle · 2020-07-23T16:22:25Z

I've just added two more commits: one two add a bootstrap and the second to add support for the pkg-config plus CPPFLAGS and LDFLAGS environment variables.

fscheiner · 2020-07-23T16:59:20Z

I just tested a uberftp compiled from the sources with this patch set applied and directly ran it from the sources directory and it behaves strange: When doing an ls on our GridFTP server, the server shows that it sent the directory listing, but uberftp does not show anything and also does not return. Hitting Ctrl+c ends uberftp and also closes the connection. The older v2.8 uberftp I created from the EPEL/Fedora sources (available from https://build.opensuse.org/package/show/home:frank_scheiner:gct/uberftp) does work as expected in this regard. Does it (v2.9-Beta) work for you?

I'm currently struggling to build an uberftp without this patch set - the configure run can't find globus_config.h for some reason - so can't really tell if this patch set is responsible. But from the changes made here I assume the problems might be coming from an older commit.

fscheiner · 2020-07-23T17:17:34Z

Ok, found the culprit: some paths in configure(.ac) were wrong for openSUSE Leap 15.1:

diff --git a/configure b/configure
index ccde4e3..539d1b6 100755
--- a/configure
+++ b/configure
@@ -4674,9 +4674,9 @@ fi
 if test "x$globus_location" == "x/usr" ; then
 	# RPM installation
 	if test "$FLAVOR" = "32"; then
-		CPPFLAGS=-I$globus_location/lib/globus/include
+		CPPFLAGS=-I$globus_location/include/globus
 	else
-		CPPFLAGS=-I$globus_location/lib64/globus/include
+		CPPFLAGS=-I$globus_location/include/globus
 	fi

Testing a uberftp compiled from the master branch gives the same result, so something between 012788f and 7bbe1ff is wrong. I should have tested these changes. :-/

fscheiner · 2020-07-23T17:51:42Z

Ok, according to bisecting and my testing, uberftp is bad since 6cd7446 (corresponding PR: JasonAlt#5). Building from 012788f gives a working version (when adapting the paths in configure.ac and doing a autoreconf -vi prior to ./configure && make), which is the state at which we forked UberFTP.

msalle · 2020-07-24T08:29:32Z

Ok, found the culprit: some paths in configure(.ac) were wrong for openSUSE Leap 15.1:

diff --git a/configure b/configure
index ccde4e3..539d1b6 100755
--- a/configure
+++ b/configure
@@ -4674,9 +4674,9 @@ fi
 if test "x$globus_location" == "x/usr" ; then
 	# RPM installation
 	if test "$FLAVOR" = "32"; then
-		CPPFLAGS=-I$globus_location/lib/globus/include
+		CPPFLAGS=-I$globus_location/include/globus
 	else
-		CPPFLAGS=-I$globus_location/lib64/globus/include
+		CPPFLAGS=-I$globus_location/include/globus
 	fi

You mean this fixes it, or this is wrong? It looks like msalle@2b7d81b but differently? Note that the latter is also superceded by my commit from yesterday.
It's always a bit tricky: the (EPEL) globus RPMs install the headers in /usr/lib(64)/include/globus, but the source installer installs them directly in PREFIX/include. If you give a --with-globus for the RPMs this therefore doesn't work smoothly. For RPMs in default location you should not need that, and you can now also use pkg-config i.e. specify a PKG_CONFIG_PATH

msalle · 2020-07-24T08:37:57Z

I just tested a uberftp compiled from the sources with this patch set applied and directly ran it from the sources directory and it behaves strange: When doing an ls on our GridFTP server, the server shows that it sent the directory listing, but uberftp does not show anything and also does not return. Hitting Ctrl+c ends uberftp and also closes the connection. The older v2.8 uberftp I created from the EPEL/Fedora sources (available from https://build.opensuse.org/package/show/home:frank_scheiner:gct/uberftp) does work as expected in this regard. Does it (v2.9-Beta) work for you?

I'm currently struggling to build an uberftp without this patch set - the configure run can't find globus_config.h for some reason - so can't really tell if this patch set is responsible. But from the changes made here I assume the problems might be coming from an older commit.

I don't see this, I've just tested my source build against the GCT 6.2.1567772254 and 6.2.1541705016 built from source installer and both using ./uberftp prometheus.desy.de ls and ./uberftp -ls gsiftp://prometheus.desy.de/ and it all just works?

fscheiner · 2020-07-30T11:54:43Z

Ok, found the culprit: some paths in configure(.ac) were wrong for openSUSE Leap 15.1:
 [...]
You mean this fixes it, or this is wrong?

The diff fixes the issue for me.

It looks like msalle@2b7d81b but differently?

This was done based on 7bbe1ff (i.e. w/o your patches applied).

Note that the latter is also superceded by my commit from yesterday.

I believe my local copy was up to date (with your latest commit) when I did the bisecting.

It's always a bit tricky: the (EPEL) globus RPMs install the headers in /usr/lib(64)/include/globus, but the source installer installs them directly in PREFIX/include. If you give a --with-globus for the RPMs this therefore doesn't work smoothly. For RPMs in default location you should not need that, and you can now also use pkg-config i.e. specify a PKG_CONFIG_PATH

Indeed.

fscheiner · 2020-07-30T12:00:10Z

I just tested a uberftp compiled from the sources with this patch set applied and directly ran it from the sources directory and it behaves strange: [...]

I don't see this, I've just tested my source build against the GCT 6.2.1567772254 and 6.2.1541705016 built from source installer and both using ./uberftp prometheus.desy.de ls and ./uberftp -ls gsiftp://prometheus.desy.de/ and it all just works?

Strange. :-| But do you also have a Globus GridFTP server to test against? I'll check against a local GridFTP server to be sure it's not related to our GridFTP server.

fscheiner · 2020-07-30T13:20:35Z

I just tested a uberftp compiled from the sources with this patch set applied and directly ran it from the sources directory and it behaves strange: [...]

I don't see this, I've just tested my source build against the GCT 6.2.1567772254 and 6.2.1541705016 built from source installer and both using ./uberftp prometheus.desy.de ls and ./uberftp -ls gsiftp://prometheus.desy.de/ and it all just works?

Strange. :-| But do you also have a Globus GridFTP server to test against? I'll check against a local GridFTP server to be sure it's not related to our GridFTP server.

@msalle
Ok, a uberftp built with this patchset works correctly with a local GridFTP server at my lab, but not with our GridFTP server at HLRS. Then I'd say, let's just pull these changes and put uberftp closer to v2.9.

fscheiner · 2020-07-30T13:26:11Z

@msalle, @matyasselmeci, @ellert:
As I created that PR - actually to save us some time - I am not allowed to perform an approving review by the GitHub UI. :-(

So @matyasselmeci or @ellert, would you be so kind and approve (and merge) these changes. From my testing with a local Globus GridFTP server and @msalle's testing with dCache they are working well.

msalle · 2020-07-31T08:39:02Z

Ok, found the culprit: some paths in configure(.ac) were wrong for openSUSE Leap 15.1:
 [...]
You mean this fixes it, or this is wrong?
The diff fixes the issue for me.

It looks like msalle@2b7d81b but differently?

This was done based on 7bbe1ff (i.e. w/o your patches applied).

you mean your diff fixes the build problems, which also should be fixed by my commits, but ...

Note that the latter is also superceded by my commit from yesterday.

I believe my local copy was up to date (with your latest commit) when I did the bisecting.

... my commit should not change anything about this run-time problem. So just to be clear: the failure to run must be caused by one of the commits we now have imported from the devel branch, hence this PR should not interfere with a potential solution of the other issue. Do you agree with that?

It's always a bit tricky: the (EPEL) globus RPMs install the headers in /usr/lib(64)/include/globus, but the source installer installs them directly in PREFIX/include. If you give a --with-globus for the RPMs this therefore doesn't work smoothly. For RPMs in default location you should not need that, and you can now also use pkg-config i.e. specify a PKG_CONFIG_PATH

Indeed.

just to make also this clear: do you have any build-problems when using my latest commits?

fscheiner · 2020-08-06T15:41:32Z

Ok, found the culprit: some paths in configure(.ac) were wrong for openSUSE Leap 15.1:
[...]

you mean your diff fixes the build problems, which also should be fixed by my commits

Yes. As I did a bisecting I had to checkout older revisions of the code, which needed some changes before I could configure and compile them on openSUSE Leap 15.1. This is not related to your changes, although they fix the same problem.

, but ...

Note that the latter is also superceded by my commit from yesterday.

I believe my local copy was up to date (with your latest commit) when I did the bisecting.

... my commit should not change anything about this run-time problem. So just to be clear: the failure to run must be caused by one of the commits we now have imported from the devel branch, hence this PR should not interfere with a potential solution of the other issue. Do you agree with that?

Yes, I totally agree. Sorry, I thought I had made that clear with #8 (comment) already, but obviously not.

It's always a bit tricky: the (EPEL) globus RPMs install the headers in /usr/lib(64)/include/globus, but the source installer installs them directly in PREFIX/include. If you give a --with-globus for the RPMs this therefore doesn't work smoothly. For RPMs in default location you should not need that, and you can now also use pkg-config i.e. specify a PKG_CONFIG_PATH

Indeed.

just to make also this clear: do you have any build-problems when using my latest commits?

No.

fscheiner · 2020-09-03T17:06:07Z

@ellert: Do you have time to perform a quick review of these changes? Because two other PRs are ready to be merged after rebased to the source with the changes from this PR included and this one's already open since end of July. Please see #8 (comment) for why.

ellert

Looks good.

msalle and others added 2 commits July 10, 2020 17:39

Update ChangeLog

fc2616d

msalle force-pushed the autotools_cleanup_rebased branch from 9b52392 to fc2616d Compare July 10, 2020 15:40

msalle requested review from ellert and matyasselmeci July 10, 2020 15:46

fscheiner commented Jul 10, 2020

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

fscheiner commented Jul 23, 2020

View reviewed changes

INSTALL Show resolved Hide resolved

msalle added 2 commits July 23, 2020 18:14

Add support pkg-config, CPPFLAGS, LDFLAGS

5e3504f

Add missing support for pkg-config, and for using environment variables CPPFLAGS and LDFLAGS. We still ignore CFLAGS which is less useful.

Adding very simple bootstrap

f11c088

Running autoreconf is sufficient to get all the autotools to work.

fscheiner mentioned this pull request Aug 20, 2020

Document requirements to support EPSV for multi-host Globus GridFTP servers #9

Merged

ellert approved these changes Sep 7, 2020

View reviewed changes

ellert merged commit 9e1737d into gridcf:master Sep 7, 2020

Conversation

fscheiner commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msalle commented Jul 10, 2020

Uh oh!

Uh oh!

Uh oh!

msalle commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fscheiner commented Jul 23, 2020

Uh oh!

fscheiner commented Jul 23, 2020

Uh oh!

fscheiner commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msalle commented Jul 24, 2020

Uh oh!

msalle commented Jul 24, 2020

Uh oh!

fscheiner commented Jul 30, 2020

Uh oh!

fscheiner commented Jul 30, 2020

Uh oh!

fscheiner commented Jul 30, 2020

Uh oh!

fscheiner commented Jul 30, 2020

Uh oh!

msalle commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fscheiner commented Aug 6, 2020

Uh oh!

fscheiner commented Sep 3, 2020

Uh oh!

ellert left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fscheiner commented Jul 9, 2020 •

edited

Loading

msalle commented Jul 23, 2020 •

edited

Loading

fscheiner commented Jul 23, 2020 •

edited

Loading

msalle commented Jul 31, 2020 •

edited

Loading