devtools: catch symbol duplicates in version map

Message ID 20210225111457.32540-1-david.marchand@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series devtools: catch symbol duplicates in version map |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot fail travis build: failed
ci/github-robot success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-mellanox-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS

Commit Message

David Marchand Feb. 25, 2021, 11:14 a.m. UTC
  Add a check on versioned symbol duplicates in map files.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
This check could be added to buildtools/check-symbols.sh so that
regular developers catch the issue when building their changes...
Opinions?

---
 devtools/check-symbol-maps.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
  

Comments

Thomas Monjalon Feb. 25, 2021, 11:41 a.m. UTC | #1
25/02/2021 12:14, David Marchand:
> Add a check on versioned symbol duplicates in map files.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> This check could be added to buildtools/check-symbols.sh so that
> regular developers catch the issue when building their changes...
> Opinions?

In general I am against adding developer tools in the build process,
because it could cause more issues for normal users.
  
Bruce Richardson Feb. 25, 2021, 11:57 a.m. UTC | #2
On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> 25/02/2021 12:14, David Marchand:
> > Add a check on versioned symbol duplicates in map files.
> > 
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > This check could be added to buildtools/check-symbols.sh so that
> > regular developers catch the issue when building their changes...
> > Opinions?
> 
> In general I am against adding developer tools in the build process,
> because it could cause more issues for normal users.
> 
Is this really likely to cause issues for normal users? If we add this to
the build process any issues will surely be caught be developers before
merge.
  
Thomas Monjalon Feb. 25, 2021, 12:01 p.m. UTC | #3
25/02/2021 12:57, Bruce Richardson:
> On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> > 25/02/2021 12:14, David Marchand:
> > > Add a check on versioned symbol duplicates in map files.
> > > 
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > > This check could be added to buildtools/check-symbols.sh so that
> > > regular developers catch the issue when building their changes...
> > > Opinions?
> > 
> > In general I am against adding developer tools in the build process,
> > because it could cause more issues for normal users.
> > 
> Is this really likely to cause issues for normal users?

Yes because some users will have a different shell,
or other weird setup we don't think about yet.

> If we add this to
> the build process any issues will surely be caught be developers before
> merge.

If we want the checks to be more popular, we should write a script
to help running the right script at the right time,
and ideally help in the contribution process.
  
Bruce Richardson Feb. 25, 2021, 12:05 p.m. UTC | #4
On Thu, Feb 25, 2021 at 01:01:10PM +0100, Thomas Monjalon wrote:
> 25/02/2021 12:57, Bruce Richardson:
> > On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> > > 25/02/2021 12:14, David Marchand:
> > > > Add a check on versioned symbol duplicates in map files.
> > > > 
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > ---
> > > > This check could be added to buildtools/check-symbols.sh so that
> > > > regular developers catch the issue when building their changes...
> > > > Opinions?
> > > 
> > > In general I am against adding developer tools in the build process,
> > > because it could cause more issues for normal users.
> > > 
> > Is this really likely to cause issues for normal users?
> 
> Yes because some users will have a different shell,
> or other weird setup we don't think about yet.
> 
I think it unlikely, but ok.

Now that meson (from version 0.53 onwards) has a "filesystem" module, with
an "exists" function, we can perhaps look to introduce a "developer mode"
build again, based off the presence of the .git folder. Alternatively, we
could make "developer mode" a regular meson option rather than trying to be
too smart about it. [Or combine both and have developer mode option with
"enabled"/"disabled"/"auto-detect" values]

/Bruce
  
Thomas Monjalon Feb. 25, 2021, 1:14 p.m. UTC | #5
25/02/2021 13:05, Bruce Richardson:
> On Thu, Feb 25, 2021 at 01:01:10PM +0100, Thomas Monjalon wrote:
> > 25/02/2021 12:57, Bruce Richardson:
> > > On Thu, Feb 25, 2021 at 12:41:16PM +0100, Thomas Monjalon wrote:
> > > > 25/02/2021 12:14, David Marchand:
> > > > > Add a check on versioned symbol duplicates in map files.
> > > > > 
> > > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > > ---
> > > > > This check could be added to buildtools/check-symbols.sh so that
> > > > > regular developers catch the issue when building their changes...
> > > > > Opinions?
> > > > 
> > > > In general I am against adding developer tools in the build process,
> > > > because it could cause more issues for normal users.
> > > > 
> > > Is this really likely to cause issues for normal users?
> > 
> > Yes because some users will have a different shell,
> > or other weird setup we don't think about yet.
> > 
> I think it unlikely, but ok.

You would be surprised.
I'm going to send a fix for running buildtools/check-symbols.sh
on busybox.

> Now that meson (from version 0.53 onwards) has a "filesystem" module, with
> an "exists" function, we can perhaps look to introduce a "developer mode"
> build again, based off the presence of the .git folder. Alternatively, we
> could make "developer mode" a regular meson option rather than trying to be
> too smart about it. [Or combine both and have developer mode option with
> "enabled"/"disabled"/"auto-detect" values]

Yes, a developer mode is a good idea.
  

Patch

diff --git a/devtools/check-symbol-maps.sh b/devtools/check-symbol-maps.sh
index c3cbcaf720..a430de7f64 100755
--- a/devtools/check-symbol-maps.sh
+++ b/devtools/check-symbol-maps.sh
@@ -35,6 +35,21 @@  if [ -n "$orphan_symbols" ] ; then
     ret=1
 fi
 
+find_duplicate_symbols ()
+{
+    for map in $(find lib drivers -name '*.map') ; do
+        ! buildtools/map-list-symbol.sh $map | \
+		sort | uniq -c | grep -v " 1 $map"
+    done
+}
+
+duplicate_symbols=$(find_duplicate_symbols)
+if [ -n "$duplicate_symbols" ] ; then
+    echo "Found duplicates in symbol map file:"
+    echo "$duplicate_symbols"
+    ret=1
+fi
+
 find_orphan_windows_symbols ()
 {
     for def in $(find lib drivers -name '*_exports.def') ; do