[R-package] Suppress 'warning: unknown pragma' for CRAN package

[jameslamb]

Description of the problem

As of the changes in #3160 , builds of the CRAN version of the R package with gcc now raise "unknown pragma" warnings.

CRAN does not allow the use of -Wunknown-pragmas to suppress such warnings, so we remove them with sed when building the R package.

The "bug" here is that LightGBM's CI did not catch this. That should also be fixed.

How to fix this

Add pragma: warning to the set of checks at

LightGBM/build-cran-package.sh

Lines 53 to 65 in 5b0ab05

	# Remove 'region' and 'endregion' pragmas. This won't change
	# the correctness of the code. CRAN does not allow you
	# to use compiler flag '-Wno-unknown-pragmas' or
	# pragmas that suppress warnings.
	echo "Removing unknown pragmas in headers"
	for file in src/include/LightGBM/*.h; do
	sed \
	-i.bak \
	-e 's/^.*#pragma region.*$//' \
	-e 's/^.*#pragma endregion.*$//' \
	"${file}"
	done
	rm src/include/LightGBM/*.h.bak

.

Add some code to the CI script for Mac/Linux that tests for the presence of this warning and fails if it finds it. Similar to

LightGBM/.ci/test_r_package.sh

Lines 206 to 216 in 5b0ab05

	if [[ $OS_NAME == "macos" ]] && [[ $R_BUILD_TYPE == "cran" ]]; then
	omp_working=$(
	cat $BUILD_LOG_FILE \
	| grep -E "checking whether OpenMP will work .*yes" \
	| wc -l
	)
	if [[ $omp_working -ne 1 ]]; then
	echo "OpenMP was not found, and should be when testing the CRAN package on macOS"
	exit -1
	fi
	fi

.

How fixing this improves {lightgbm}

This warning does not affect the correctness of an installation, but showing it in logs may waste users' time by leading them to believe that something is wrong with their installation.

Environment info

LightGBM version or commit hash: latest master (5b0ab05)

Error message and / or logs

clang++ -mmacosx-version-min=10.13 -std=gnu++11 -I"/Library/Frameworks/R.framework/Resources/include" -DNDEBUG -I./include -DUSE_SOCKET -DLGB_R_BUILD  -I/usr/local/include  -Xclang -fopenmp -pthread -fPIC  -Wall -g -O2  -c treelearner/voting_parallel_tree_learner.cpp -o treelearner/voting_parallel_tree_learner.o
In file included from treelearner/voting_parallel_tree_learner.cpp:11:
In file included from treelearner/parallel_tree_learner.h:15:
treelearner/cuda_tree_learner.h:255:13: warning: unknown pragma ignored [-Wunknown-pragmas]
    #pragma warning(disable : 4702)
            ^
1 warning generated.

Reproducible example(s)

On a Mac, using gcc:

sh build-cran-package.sh

R CMD INSTALL lightgbm_*.tar.gz

[cctechwiz]

I'll take this one. I don't have a Mac like the example, but I do have Linux w/ gcc so that should work the same.

[jameslamb]

sure, thanks for contributing! I'll assign it to you.

I agree, this is probably a gcc-specific thing, not a Mac-specific thing. If you get stuck with our testing setup, let me know!

[cctechwiz]

So I guess I was wrong and it might be more Mac-specific than I thought. I am unable to see the gcc warnings when I build.

Versions:

$ gcc --version
gcc (Debian 10.2.0-13) 10.2.0

$ R --version
R version 4.0.3 (2020-10-10) -- "Bunny-Wunnies Freak Out"

That being said, I think I can still make the change need and verify them in other ways.

Before my changes to build-cran-package.sh

$ grep -RIi "pragma warning" ./lightgbm_r/

./lightgbm_r/src/c_api.cpp:  #pragma warning(disable : 4702)
./lightgbm_r/src/c_api.cpp:  #pragma warning(disable : 4702)
./lightgbm_r/src/include/LightGBM/c_api.h:  #pragma warning(disable : 4996)
./lightgbm_r/src/include/LightGBM/meta.h:  #pragma warning(disable : 4127)
./lightgbm_r/src/include/LightGBM/utils/openmp_wrapper.h:  #pragma warning(disable : 4068)  // disable unknown pragma warning
./lightgbm_r/src/treelearner/cuda_tree_learner.h:    #pragma warning(disable : 4702)
./lightgbm_r/src/treelearner/gpu_tree_learner.h:    #pragma warning(disable : 4702)

After my changes to build-cran-package.sh

$ grep -RIi "pragma warning" ./lightgbm_r/

./lightgbm_r/src/c_api.cpp:  #pragma warning(disable : 4702)
./lightgbm_r/src/c_api.cpp:  #pragma warning(disable : 4702)

Should the cpp files also be combed for pragmas?

Let me know if you think I should proceed with change / PR or if you think someone else should take this issue who can better replicate your original issue?

[cctechwiz]

I forgot to include a reference to my fork with the changes mentioned above:

https://github.com/cctechwiz-forks/LightGBM/tree/suppress-warning-unknown-pragma-cran

I can make a PR if that is easier, I just didn't want to go forward with that if you'd rather have someone else test this.

[jameslamb]

thanks @cctechwiz !

Yes, we should also remove these warning-suppression pragmas from .cpp and .hpp files.

Please open a PR. We'll be able to see the results from Mac builds in CI there. Please check the box on it that says "allow updates by maintainers", and I'll be able to push to it as well.

[cctechwiz]

Sounds good.

[cctechwiz]

I did not see a checkbox for "allow updates by maintainers" while making this PR. I've seen that checkbox before, so maybe I just missed it? Are there certain criteria that you are aware of for allowing maintainers to update?

[jameslamb]

It looks like this, whenever you click "Create pull request" (see the bottom left corner of the image below)

Are there certain criteria that you are aware of for allowing maintainers to update?

That "allow maintainers to update" is just a thing that would allow me to push changes directly to your branch in your fork, but only while it is in an open pull request against the upstream project. So it's a much tighter set of permissions than you adding me as a contributor on cctechwiz/LightGBM.

Not a problem though, it looks like maybe it defaults to true on GitHub, and I don't know if we'll even end up needing it.

[github-actions]

This issue has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.