Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

notcurses: build on Linux #85160

Closed
wants to merge 1 commit into from
Closed

Conversation

carlocab
Copy link
Member

Using GCC doesn't work, so let's try Clang.

@carlocab carlocab added CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Sep 14, 2021
@BrewTestBot BrewTestBot added the no Linux bottle Formula has no Linux bottle label Sep 14, 2021
@carlocab carlocab added CI-no-bottles Merge without publishing bottles build failure CI fails while building the software help wanted Task(s) needing PRs from the community or maintainers linux Linux is specifically affected labels Sep 14, 2021
@carlocab
Copy link
Member Author

@dankamongmen do these errors mean anything to you? Do we just have the wrong C++ standard set, or something?

@dankamongmen
Copy link
Contributor

@dankamongmen do these errors mean anything to you? Do we just have the wrong C++ standard set, or something?

std::is_same is c++2017 (https://en.cppreference.com/w/cpp/types/is_same), and as CMake specifies, we do indeed need a C++17 compiler. CMake ought be adding -std=c++17 to the invocation arguments.

@Bo98
Copy link
Member

Bo98 commented Sep 14, 2021

This still uses GCC 5 C++ headers, that's why.

I'd recommend using the latest GCC here - what's the error with that? Otherwise we would need to switch entirely to libc++.

@carlocab
Copy link
Member Author

In file included from /usr/include/string.h:630,
                 from /tmp/notcurses-20210914-22292-1761r02/notcurses-2.4.1/src/lib/direct.c:6:
/tmp/notcurses-20210914-22292-1761r02/notcurses-2.4.1/src/compat/compat.h:82:7: error: expected identifier or '(' before '__extension__'
   82 | char* strndup(const char* str, size_t size);
      |       ^~~~~~~
CMakeFiles/notcurses-core.dir/build.make:120: recipe for target 'CMakeFiles/notcurses-core.dir/src/lib/direct.c.o' failed
make[2]: *** [CMakeFiles/notcurses-core.dir/src/lib/direct.c.o] Error 1

https://github.com/Homebrew/homebrew-core/pull/85148/checks?check_run_id=3597979801#step:7:327

I guess it's still the same problem as here.

@Bo98
Copy link
Member

Bo98 commented Sep 14, 2021

Looks like that error didn't disappear with Clang.

strndup is a #define on our Ubuntu installation so overriding that can just break the syntax. If overriding it is the intention, it looks like a #undef strndup is needed - this is what glibc does.

@carlocab
Copy link
Member Author

carlocab commented Sep 14, 2021

That just reads like a forward declaration and not an override to me? Oh wait no, I just saw compat.c.

@Bo98
Copy link
Member

Bo98 commented Sep 14, 2021

Well, it's a header and there's a .c file we've not yet reached but the point still stands.

From our glibc: https://github.com/bminor/glibc/blob/glibc-2.23/string/bits/string2.h#L1295-L1321

This would result in the declaration being:

char* (__extension__ (__builtin_constant_p (const char* str) && __string2_1bptr_p (const char* str) \
		  ? (((const char *) (const char* str))[0] == '\0'	      \
		     ? (char *) calloc ((size_t) 1, (size_t) 1)		      \
		     : ({ size_t __len = strlen (const char* str) + 1;	      \
			  size_t __n = (size_t size);			      \
			  char *__retval;				      \
			  if (__n < __len)				      \
			    __len = __n + 1;				      \
			  __retval = (char *) malloc (__len);		      \
			  if (__retval != NULL)				      \
			    {						      \
			      __retval[__len - 1] = '\0';		      \
			      __retval = (char *) memcpy (__retval, const char* str, \
							  __len - 1);	      \
			    }						      \
			  __retval; }))					      \
		  : __strndup (const char* str, size_t size)));

Which is obviously not what is desired.

The fix would be to do one of:

  • Use one available in the system if available
  • Call it something else
  • Undef strndup (if this header isn't exposed externally).

@carlocab
Copy link
Member Author

What happens if we define _HAVE_STRING_ARCH_strndup?

@Bo98
Copy link
Member

Bo98 commented Sep 14, 2021

Sounds like a hack to a problem that should probably be resolved upstream.

@dankamongmen
Copy link
Contributor

Sounds like a hack to a problem that should probably be resolved upstream.

indeed; if y'all want to use these compilers, let me go get things working with these compilers.

@Bo98
Copy link
Member

Bo98 commented Sep 14, 2021

I'd say you will probably manage to reproduce the issue with Ubuntu 16.04. Using the latest GCC (11) is fine, and necessary for C++17. glibc version is the main blocker here and breaks regardless of the compiler version.

@carlocab
Copy link
Member Author

Let's revisit this later.

@carlocab carlocab closed this Oct 30, 2021
@carlocab carlocab deleted the notcurses-linux branch October 30, 2021 05:23
@dankamongmen
Copy link
Contributor

hey there, sorry, forgot all about this. i'd love to build in Linuxbrew. if you'd be willing, @carlocab (or anyone else), please file an issue on Notcurses explaining what needs done, and i'll make sure it happens, ideally before the 3.0.0 release coming up this November.

@carlocab
Copy link
Member Author

Well, part of the reason I've closed this is 1) I'm not super clear on what needs to be done (which makes filing an issue tricky), and 2) I suspect the problem will go away when we start building on Ubuntu 18.04 (migration should happen at some point soon).

Point 2 above in particular means we might be able to get this built on Linux by just waiting a little bit.

@carlocab carlocab mentioned this pull request Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build failure CI fails while building the software CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. CI-no-bottles Merge without publishing bottles CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. help wanted Task(s) needing PRs from the community or maintainers linux Linux is specifically affected no Linux bottle Formula has no Linux bottle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants