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

__time64_t and 32bit platform #563

Closed
yopito opened this issue Jul 5, 2019 · 7 comments
Closed

__time64_t and 32bit platform #563

yopito opened this issue Jul 5, 2019 · 7 comments

Comments

@yopito
Copy link
Contributor

yopito commented Jul 5, 2019

while packaging passwordsafe 1.08.1BETA, build fails for 32bit platform.

Context: VoidlLinux distribution with gcc 9.1 and glibc 2.29 or musl libc 1.1.22

Example of fail build: https://travis-ci.org/void-linux/void-packages/jobs/535496599e

Offending starts at is https://github.com/pwsafe/pwsafe/blob/master/src/os/unix/pws_time.h#L18

my points:

  • "__time64_t" typedef is defined on 32bit platform, even if "__time64_t" define statement is not
  • fill confused that "__time64_t" typedef is defined in pws_time.h, that is a structure defined in libc headers (see [1])
  • libc header (see [1]) also states that "__time64_t" is always a 64bit type (via __TIME64_T_TYPE)

Fixed myself build via replacing L18 with
#if !defined(__time64_t) && !defined(__TIME64_T_TYPE)
It's working, but I'm not sure this is the right way to fix it

So pws_time.h should not redefine __time64_t or use another name for that ?
I'm not enough fluent in C/C++ to propose a good patch, sorry.

[1] Gnu libc include/bits/time64.h states:

26  /* Define __TIME64_T_TYPE so that it is always a 64-bit type.  */
27
28  #if __TIMESIZE == 64
29  /* If we already have 64-bit time type then use it.  */
30  # define __TIME64_T_TYPE                __TIME_T_TYPE
31  #else
32  /* Define a 64-bit time type alongsize the 32-bit one.  */
33  # define __TIME64_T_TYPE                __SQUAD_TYPE
34  #endif
35
36  #endif /* bits/time64.h */
@ronys
Copy link
Member

ronys commented Jul 5, 2019

Fixed in commit 497246d. I took your fix, and removed the unused __time32_t typedef.

It's not that pwsafe is redefining __time64_t, but rather defining it for environments where it's missing, making the rest of the code more consistent.

@ronys ronys closed this as completed Jul 5, 2019
@yopito
Copy link
Contributor Author

yopito commented Jul 5, 2019

cool, quick response.
FYI, with your fix, build with various arch and libc combinations is fine (voidlinux).
See https://travis-ci.org/void-linux/void-packages/builds/554874381 for instance

@ronys
Copy link
Member

ronys commented Jul 6, 2019

Nice. Interesting to see builds for ARM as well.

Care to submit a pull request to integrate voidlinux support into the main repo?

@yopito
Copy link
Contributor Author

yopito commented Jul 6, 2019

Thanks for your interrest to VoidLinux, but I don't well understand how it could be achieved in pwsafe context: specific entry in CmakeList.txt ? Voidlinux as travis target ?

@ronys
Copy link
Member

ronys commented Jul 6, 2019

  • Well, if you made any changes to the source code besides the fix we've discussed, that would be of interest.
  • If cmake supports voidlinux packages, then changes to CmakeList.txt would be relevant
  • If the voidlinux package can be built by a script on a voidlinux system, then that script can be added under the install directory in the project repo.
  • Adding voidlinux to the existing travis.yml file would also be useful, I think.

@yopito
Copy link
Contributor Author

yopito commented Jul 11, 2019

sorry for late reply. It feels difficult to answer you clearly, so here my points.

VoidLinux distributes passwordsafe as an official package:

To achieve packaging, VoidLinux uses a integrated tree that contains both the source packaging definition of all packages and the shell build utility and its helpers (xbps-src).
So:

  • The packaging of a given application has to be maintained in that tree
  • I could provide you a copy of the templating file for passwordsafe, but it sounds a bit useless since template directives heavily rely on the build ecosystem that is constantly evolving (this is a rolling distribution). Difficult to maintain ?
  • Travis does not offer a VoidLinux image. VoidLinux uses travis on top of Ubuntu image with a static package manager and retrieve the combined tree to launch test packaging. Official build is achieved on dedicated systems of the project. So using VoidLinux as a QA target for passwordsafe can be tricky ?

Anyway, I will be glad to help you on any of these points if you want to move on.

@ronys
Copy link
Member

ronys commented Jul 11, 2019

Thanks for clarifying. Based on your explanation, I don't see any real value in adding voidlinux specifics to the main passwordsafe repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants