Building with seperate SOURCE and BUILD directories npa-tool.ggo issues

[dengert]

Expected behaviour

npa-tool.ggo needs to be written to either SOURCE or BUILD so it can be read from that directory.
Problem started today after git pull of master.

Actual behaviour

SOURCE = /afs/anl.gov/appl/OpenSC-dev/build/opensc-git-my/OpenSC
BUILD = /afs/anl.gov/appl/OpenSC-dev/build/opensc-git-my/amd64_linux26-1.0.1
../src/configure
--disable-silent-rules
--prefix=$PREFIX (plus other options not part of this problem)

make then has these errors:

make[3]: Entering directory `/afs/anl.gov/appl/OpenSC-dev/build/opensc-git-my/amd64_linux26-1.0.1/src/tools'

/bin/sed -e 's,[@]CVCDIR[@],,g' -e 's,[@]PACKAGE[@],opensc,g' -e 's,[@]PACKAGE_BUGREPORT[@],[email protected],g' -e 's,[@]PACKAGE_NAME[@],Open SC,g' -e 's,[@]PACKAGE_TARNAME[@],opensc,g' -e 's,[@]PACKAGE_URL[@],,g' -e 's,[@]PACKAGE_SUMMARY[@],,g' -e 's,[@]PACKAGE_VERSION[@],"0.16.0",g' -e 's,[@]X509DI R[@],,g' < ../../../src/src/tools/npa-tool.ggo.in > ../../../src/src/tools/npa-tool.ggo

/usr/bin/gengetopt --output-dir=../../../src/src/tools < npa-tool.ggo

/bin/bash: npa-tool.ggo: No such file or directory

make[3]: *** [cmdline.h] Error 1

make[3]: Leaving directory `/afs/anl.gov/appl/OpenSC-dev/build/opensc-git-my/amd64_linux26-1.0.1/src/tools'

The problem is:
/bin/sed writes ../../../src/src/tools/npa-tool.ggo into SOURCE
/usr/bin/gengetopt tried to read npa-tool.ggo from the current BUILD

Where should it be written?

What needs to be changed here. The help2man looks like it will also have a problem.

./src/tools/Makefile.in
1171 $(BUILT_SOURCES): npa-tool.ggo
1172 $(GENGETOPT) --output-dir=$(srcdir) < $<
1173
1174 npa-tool.ggo: npa-tool.ggo.in
1175 $(do_subst) &lt; $&lt; > $(srcdir)/$@
1176
1177 $(abs_builddir)/npa-tool.1: npa-tool.ggo.in
1178 $(HELP2MAN)
1179 --output=$@
1180 --no-info
1181 --source='$(PACKAGE_STRING)'
1182 $(builddir)/npa-tool

Steps to reproduce

1. Use separate build and source directories.

[dengert]

@frankmorgner can you look at this.
I don't have OPENPACE (and not interested right now is using it or npa-tool)

npa-tool.c has #ifdef ENABLE_OPENPACE and if not defined program returns -1.
All the ./src/tools/Makefile.in lines listed above are not needed if npa-tool is a dummy module.
All the SOURCE vs BUILD directories issues are in the same same lines in ./src/tools/Makefile.in
They still need to be fixed if OPENPACE is available.

Circumvention

So for now, I am sticking with: issue823-hack-no-openpace.diff.txt that modifies configure.ac to not build npa-tool if openpace is not found.

[viktorTarasov]

same subject in #822

[viktorTarasov]

@frankmorgner

I propose you to:

• force/rebase the 'master' and return it to the state before 'NPA/PACE' (or exclude the NPA/PACE commits);
• rebase your NPA/PACE contribution to the one/two commits -- you are introducing a new card driver, no one is interested in the history of your doubts, frustrations, search of yourself; so, please do not import all this garbage into the 'master' history.
• take care of existing source layout;
• take care of adopted coding style.

[dengert]

+1
Also see #827 comments. I agree with @viktorTarasov comments above, and the configure of npa-tool is non-standard that will cause problems in the future. Distro builders will be complaining about it, and may not port OpenSC to their systems until it is fixed.

[frankmorgner]

Hmm, the PR was open more than half a year and I announced the merged more than two weeks ago. I would have loved to get these comments earlier.

I'll look into gengetopt later this week. @viktorTarasov if you are more specific about the source layout and the coding style I'll have a look at this as well.

[viktorTarasov]

@frankmorgner

I'll look into gengetopt later this week.

The current master is broken. If you have no time, I can revert it for you.

about the source layout

It's strange that I have to explain it to you -- the card specific stuff has to be placed into the dedicated folders. If you are not agree -- expose your arguments.

coding style

"use tabs, no space"

[dengert]

New card drivers don't normally make major change to the
configure.ac and and require additional packages... OPENPACE,
help2man, gengetopt. 
I would have expected to this to have been tested better, or at
least you asked for other too try it. If you did do these things, we
all must have missed it. A lack of a response is not the same as an
OK. 

On 7/11/2016 7:46 AM, Frank Morgner
  wrote:


  Hmm, the PR was open more than half a year and I announced the
    merged more than two weeks ago. I would have loved to get these
    comments earlier.
  I'll look into gengetopt later this week. @viktorTarasov if you are more
    specific about the source layout and the coding style I'll have
    a look at this as well.
  —
    You are receiving this because you authored the thread.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.









-- 

Douglas E. Engert [email protected]

[frankmorgner]

Please calm down everyone. Even if there are any problems, I think those can be resolved leaving nobody behind.

It's true that master got an additional dependency, which was gengetopt (not OpenPACE). This should be fixed with 9883446 (one day after report); OpenSC now builds with its usual dependencies.

I'm still not sure what source layout you'd prefer, @viktorTarasov.

• libcardnpa is an external card driver library and has its own folder
• libisosm implements SM encoding according to ISO 7816, which means it sits between a card driver (libopensc) and libsm (which has its own folder). It can (and, in my opinion, should) be used by other ISO compliant card drivers to reduce the overall code base.
• libsceac establishes an SM channel according to EACv2 and has its own folder. EAC is used by a handfull of european ID cards, new MRTDs, and, in fact, I'm using it right now to create a SM channel to sc-hsm (about to be published). So it's not card specific.

I don't think that we need to fight over some folder structure. Just choose what you like.

Regarding the indenting, please see #826.
Regarding the build directories, please see #827.

[viktorTarasov]

@frankmorgner
9883446 -- good action. What about source layout and master history?

[frankmorgner]

I don't think it's possible to change or rewrite the history in master anymore. This would basically destroy the git history of everyone who checked it out after the merge. After all, there are only a handfull of fixups that would disappear. Believe me, there were many more iterations to get the full blown and robust implementation we have now. The full history is available here https://github.com/frankmorgner/vsmartcard/tree/master/npa.

I tried to explain the reasoning behind the source layout above (#823 (comment)). Do you see a problem with this?

[viktorTarasov]

I don't think it's possible to change or rewrite the history in master anymore ...

For you we will do an exception.

[frankmorgner]

With #611 I merged 20 commits, from which 10 are fixups that could be squashed. Please look at the OpenSC history we have now, there are a ton of fixups and many bad commit messages that piled up in the past. Are you really, really sure that you want to destroy any local copy of the repository just for kicking out 10 lines in the git history?

[viktorTarasov]

As for the source layout :

• join the libisosm with libsm ;
• move eac, npa cards/stuff into libopensc, (probably create dedicated sub-folder inside libopensc);
• think about some common denominator for the name of pace, boxing, npa, sceac source files (something like xxx-mrtd-yyy, not sure)

I still maintain my proposal to rebase master -- rebase your contribution, remove merge of #795, probably something else. Think about using 'sc_log' instead of 'sc_debug' ...

For the future -- no dumb merges for the verbose PRs: author or committer has to rebase such PRs to the reasonably minimal number of commits.

[frankmorgner]

no dumb merges

I guess you meant something like a silent merge, right?

[viktorTarasov]

no dumb merges

I guess you meant something like a silent merge, right?

I mean merge with the merge-PR button of github interface, without any regards onto the history of the branch to be merged .

[viktorTarasov]

@frankmorgner
I do not seen your readiness to change the npa-centric source layout, that you have recently commited to master, so,
I force/rebased master excluding the npa merge.

The previous master is saved as master-with-mrtd.

We can start the discussion around your mrtd contribution from the begining.