[Openmp-commits] [PATCH] D11494: test-release.sh: Add -openmp option.

[Renato Golin]

rengolin added a comment.

Hi Hans,

Sorry, this fell out of my radar. I think it's an important addition, I just think it should be part of the main build and behave like the others. I have left some comments inline.

Also, about the tar ball, are we going to start uploading both of them? Or is it maybe better to bundle the OpenMP tar inside the LLVM+Clang one? If we make this part of the main build, it may be easier to bundle all together.

Either way is fine for me, but would be good if all target testers / code owners would agree on one for a reason. :)

cheers,
--renato


================
Comment at: utils/release/test-release.sh:440
@@ +439,3 @@
+    cwd=`pwd`
+    set -x  # Echo on.
+
----------------
"set -x" considered harmful. :)

================
Comment at: utils/release/test-release.sh:442
@@ +441,3 @@
+
+    rm -rf $BuildDir/openmp
+    rm -rf $BuildDir/openmp.install
----------------
This better stay in PhaseM/Release/... like the rest.

================
Comment at: utils/release/test-release.sh:446
@@ +445,3 @@
+    cd $BuildDir/openmp
+    clang=$BuildDir/Phase3/Release/llvmCore-$Release-$RC.install/usr/local/bin/clang
+    cmake -DCMAKE_C_COMPILER=${clang} -DCMAKE_CXX_COMPILER=${clang}++ \
----------------
Hasn't this moved out of /usr/local?

================
Comment at: utils/release/test-release.sh:454
@@ +453,3 @@
+
+    local Package=OpenMP-$Triple
+    mv $BuildDir/openmp.install/usr/local $BuildDir/$Package
----------------
Better not to override a global variable with the same name, it's confusing.

Call it OpenMPPackage or something.

================
Comment at: utils/release/test-release.sh:455
@@ +454,3 @@
+    local Package=OpenMP-$Triple
+    mv $BuildDir/openmp.install/usr/local $BuildDir/$Package
+    cd $BuildDir
----------------
I think this should be inside the main build process, so that we use the same standard openmp-3.7.0-install and then rename / tar like llvmCore-3.7.0-install, etc.


http://reviews.llvm.org/D11494