diivm / pcl

Point Cloud Library (PCL)
http://www.pointclouds.org
Other
0 stars 0 forks source link

Changes for module integration and build pipeline #25

Open diivm opened 4 years ago

diivm commented 4 years ago

Summary of changes:

  1. Added generate_compile_commands.py for:

    • getting the file list from PCL's source tree, subject to a module-based whitelist.
    • generating compile commands database according to it
  2. Rearranging: bindings/python/* → python/*. Initial structure was to keep clang-tooling and pybind gen in separate folders, but is not needed now. Also, better for make python.

  1. Cleanup:
    • Removed archived folders, which were just kept for reference:
      • common-archive: had initial prototype code for manual pybind gen, etc.
      • tooling-archive: Dockerfile, Makefile, old compile_commands.json, old libclang.py, README, old parse.cpp, etc.
    • compile_commands.json, since it is generated via a python script; point_types.json removing from git
kunaltyagi commented 4 years ago

Brief summary of the changes please

kunaltyagi commented 4 years ago

Provide a brief summary as well as rationale please

diivm commented 4 years ago

Provide a brief summary as well as rationale please

Sure, sorry for the delay

kunaltyagi commented 4 years ago

Please split into 2 PRs:

The first one has lots of noise which is not needed to review code

kunaltyagi commented 4 years ago

If you have dirty commits (touching files in both the above categories), you can split the commits per file by starting an interactive rebase, marking the commit to split as editable and then running this script:

#!/usr/bin/env bash

message="$(git log --pretty=format:'%s' -n1)"

if [ `git status --porcelain --untracked-files=no | wc -l` = 0 ]
then
   git reset --soft HEAD^
fi

git status --porcelain --untracked-files=no | while read status file
do
   echo $status $file

   if [ "$status" = "M" ]
   then
      git add $file
      git commit -n $file -m "$file: $message"
   elif [ "$status" = "A" ]
   then
      git add $file
      git commit -n $file -m "added $file: $message"
   elif [ "$status" = "D" ]
   then
      git rm $file
      git commit -n $file -m "removed $file: $message"
   else
      echo "unknown status $file"
   fi
done

Once there are no dirty commits, just create a new branch from this PR, and during interactive rebase pick and choose the commits to keep.

diivm commented 4 years ago

If you have dirty commits ...

@kunaltyagi Nice script, but wasn't needed. No dirty commits.

Separated.

kunaltyagi commented 4 years ago

I can see scope for other bindings (based on survey) and it makes sense to have the json part common. Based on this, should we undo the move from bindings/python to python? Or redo it later when (and if) other bindings are added?

bindings
|-- parser.py
|-- python
|   |-- generator.py
|   |-- packaging/
|-- other-lovely-language
    |-- generator.py
    |-- packaging/
diivm commented 4 years ago

I can see scope for other bindings (based on survey) and it makes sense to have the json part common. Based on this, should we undo the move from bindings/python to python? Or redo it later when (and if) other bindings are added?

bindings
|-- parser.py
|-- python
|   |-- generator.py
|   |-- packaging/
|-- other-lovely-language
    |-- generator.py
    |-- packaging/

Taking into consideration #37, can we just move parse.py into the root later on? (So that make python and make other-lovely-language can be separate)

diivm commented 4 years ago

I think we can keep the current bindings/python structure for now. It can be dealt with later on if required. I can revert those commits and then split this into 2 PRs: generate_compile_commands.py and changes to CMakeLists.txt. What say, reviewers?

aPonza commented 4 years ago

Deal with the structure later, as Kunal also suggested. Also I think you can merge this as is, it's already thoroughly reviewed, no need to split after the work is done, what you might want to do is squash some commits together if you believe that makes them more descriptive (keep the partials in the commit message after three main message)

kunaltyagi commented 4 years ago

1 small suggested edit (and the conflicts) remains. Deal with the structure later