Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
Merged #1325 into master. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#event-3718116926___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@pmatilai approved this pull request. Works as promised AFAICT -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#pullrequestreview-480527251___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@pmatilai Updated, please review. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-684947396___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@dmnks pushed 2 commits. 82c5af992ed87bb6665de2d382166a563cc7b398 Check for OpenMP version at configure time c3af4801917c6cf3d5b5153a02f4cc09f98d6ca2 Bump Lua to 5.2 in configure script -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325/files/b1ef1a1326f2e777ea43790a79ea5fa640fbb521..c3af4801917c6cf3d5b5153a02f4cc09f98d6ca2 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
> The right thing to do with an incompatible OpenMP is to silently disable > OpenMP unless explicitly requested by --enable-openmp. Whether it's worth the > trouble is a separate question, writing configure.ac logic is ... yeah. No > cute kittens will be harmed if we set the default to "yes" instead of > implementing "auto". Nah, the logic shouldn't be much of a problem I think. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-684552367___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
Optimally, ./configure without specific arguments is supposed to just figure out a configuration that will work on this system, and only error out if it cannot do that - or is explicitly requested to do incompatible things. The right thing to do is to silently disable OpenMP unless explicitly requested by --enable-openmp. Whether it's worth the trouble is a separate question, writing configure.ac logic is ... yeah. No cute kittens will be harmed if we set the default to "yes" instead of implementing "auto". -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-684517393___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@pmatilai Coming back to this PR after a while, I wonder if silently disabling OpenMP (if the required version isn't available) is really what we want. Wouldn't it be better to just fail and let the user disable OpenMP explicitly with `--disable-openmp` if he/she decides so? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-684476437___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
Thanks, will tweak the PR accordingly. And yeah, I agree otherwise. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-675362444___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
Took more than one night of sleeping over, but here's what I think: Threading support in general is completely optional, and lack of it doesn't deprive anybody of any functionality, just a little bit of time. This is stark contrast to, say, Lua support. So I think we can easily afford to demand a newer version here than we could do for some other feature. Lets go with requiring OpenMP 4.5, it'll simplify other things in turn. The configure check might want a little tweak though: default to autodetection, and only error out if explicit --enable-openmp is given but OpenMP is too old. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-675326483___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@Conan-Kudo commented on this pull request. > @@ -761,9 +778,9 @@ AC_ARG_WITH([lua], [AS_HELP_STRING([--with-lua], [build > with lua support])], AS_IF([test "$with_lua" != no],[ PKG_CHECK_MODULES([LUA], -[lua >= 5.1], +[lua >= 5.2], Hah, I thought we actually dropped Lua 5.2 support as well. Okay then... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#discussion_r466969467___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@dmnks commented on this pull request. > @@ -761,9 +778,9 @@ AC_ARG_WITH([lua], [AS_HELP_STRING([--with-lua], [build > with lua support])], AS_IF([test "$with_lua" != no],[ PKG_CHECK_MODULES([LUA], -[lua >= 5.1], +[lua >= 5.2], Heh, it's funny how easy is to misread the subject line of that commit message when shown in a popup box after hovering over it. It says "drop support for Lua < 5.2" with a line break after the "<" symbol which is then easy to miss :) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#discussion_r466893351___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
> Sometimes it's better to test for specifics features, sometimes for versions. > I don't know how the OpenMP landscape looks like, but sometimes > implementations only support a subset of a newer standard in which case > testing for specific features is the friendlier way. My impression after skimming through a couple of random discussion threads and stackoverflow posts is very much this; while the API is well-defined, the reality is that compilers may only have partial support for certain features. With GCC, I think it's safe to assume it's fully supported, but I agree that testing for the particular feature is always the safest option. > OTOH the simplicity of being able to say "we require version X of standard Z" > can be a bliss - for example we generally require POSIX.1 >= 2001 and that > makes it fairly easy to cross-check portability issues and to say "no" to > obscure stuff that doesn't fulfil that basic requirement. Yup, I basically had the same mindset and just went with the version check, instead of checking for the priority feature alone. That also "scales" better in case we adopt some other OpenMP features in the future; instead of adding another convoluted compilation test to the configure script, we would just bump the version. That being said, especially given the nature of the non-100% compiler support, doing something like you suggested above with a custom `HAVE_OMP_PRIORITY` flag which we would set in a compilation test in `configure.ac` seems to be the best solution here. It just doesn't feel right to mandate version 4.5 (being "too new" in terms of RPM as you noted) just because of that damn priority thing which the world can live without just fine. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-670390942___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
Just for a data point, OpenMP 4.5 is supported in clang >= 7 and gcc >= 6, both from 2018. Which is brand new software barely off its wrappings in terms of rpm software requirements :smile: -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-670377085___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
> A more user-friendly way of dealing with this would actually be the opposite, > i.e. making the use of the priority keyword conditional at preprocessing, > based on the detected OpenMP version (which is trivial to do as shown in the > patch) because as you say, all that the keyword really does is it improves > load balance a tiny little bit in specific circumstances, but its absence has > no effect on the correctness of the code. We could even avoid doing a > `qsort()` on the package list as part of that conditional. The less code is covered by conditionals the better. Another option would be to specifically test for priority support in OpenMP, and just do something like this in pack.c: ``` #ifndef HAVE_OMP_PRIORITY #define priority(x) #endif ``` Sometimes it's better to test for specifics features, sometimes for versions. I don't know how the OpenMP landscape looks like, but sometimes implementations only support a subset of a newer standard in which case testing for specific features is the friendlier way. OTOH the simplicity of being able to say "we require version X of standard Z" can be a bliss - for example we generally require POSIX.1 >= 2001 and that makes it fairly easy to cross-check portability issues and to say "no" to obscure stuff that doesn't fulfil that basic premist. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-670372444___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@pmatilai commented on this pull request. > @@ -761,9 +778,9 @@ AC_ARG_WITH([lua], [AS_HELP_STRING([--with-lua], [build > with lua support])], AS_IF([test "$with_lua" != no],[ PKG_CHECK_MODULES([LUA], -[lua >= 5.1], +[lua >= 5.2], Because 5.2 is the actually required minimum? See commit 6d4c68ba10b9713f3e599cf20c2455eb80e9bfe1 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#discussion_r466860173___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
@Conan-Kudo requested changes on this pull request. > @@ -761,9 +778,9 @@ AC_ARG_WITH([lua], [AS_HELP_STRING([--with-lua], [build > with lua support])], AS_IF([test "$with_lua" != no],[ PKG_CHECK_MODULES([LUA], -[lua >= 5.1], +[lua >= 5.2], Why aren't we just bumping to Lua 5.3? That's what we _actually_ test with and our current compatibility base is with. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#pullrequestreview-462988129___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
That's a very valid point and one that I didn't consider, honestly. A more user-friendly way of dealing with this would actually be the opposite, i.e. making the use of the priority keyword conditional at preprocessing, based on the detected OpenMP version (which is trivial to do as shown in the patch) because as you say, all that the keyword really does is it improves load balance a tiny little bit in specific circumstances. We could even avoid doing a `qsort()` on the package list as part of that conditional. Would that be better? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-669953999___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
Looks okay as such, I guess the question here is whether it's reasonable to disable all openmp-stuff if "priority" is not supported - that's required by one place only, and even that's just an optimization that doesn't actually affect correctness (AIUI). On one hand this seems a bit harsh, on the other hand having a clear base-level of what is required makes it easy to answer "can I use feature X in the codebase", without having to care and create compat foo. I'm leaning towards the latter but maybe I need to sleep over it. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325#issuecomment-669889298___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] OpenMP & Lua fixes for configure.ac & INSTALL (#1325)
You can view, comment on, or merge this pull request online at: https://github.com/rpm-software-management/rpm/pull/1325 -- Commit Summary -- * Check document need for correct OpenMP version * Bump Lua to 5.2 in configure script -- File Changes -- M INSTALL (6) M configure.ac (25) -- Patch Links -- https://github.com/rpm-software-management/rpm/pull/1325.patch https://github.com/rpm-software-management/rpm/pull/1325.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1325 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
