Skip to content

import&copy: keep the original access and modified timestamps#12367

Merged
TurboGit merged 1 commit intodarktable-org:masterfrom
phweyland:import-copy-timestamps2
Sep 6, 2022
Merged

import&copy: keep the original access and modified timestamps#12367
TurboGit merged 1 commit intodarktable-org:masterfrom
phweyland:import-copy-timestamps2

Conversation

@phweyland
Copy link
Copy Markdown
Contributor

@phweyland phweyland commented Aug 22, 2022

partially fixes #12355

EDITED
The lazy one: on windows the code is still to be written

Replaces #12357

@TurboGit
Copy link
Copy Markdown
Member

@phweyland : Have you tested on Windows? Just asking as the CI seems highly confused with Windows build at the moment and I'm not a Windows guy, so cannot test on my side. TIA.

@TurboGit TurboGit added this to the 4.0.1 milestone Aug 22, 2022
@TurboGit TurboGit added scope: DAM managing files, collections, archiving, metadata, etc. release notes: pending labels Aug 22, 2022
@phweyland
Copy link
Copy Markdown
Contributor Author

Have you tested on Windows?

No, I cannot neither.
But CI gives a new issue utimes() is not available (on windows I guess). But utime() should exists. I do it.

@TurboGit
Copy link
Copy Markdown
Member

Ok, so please if a Windows user can help here by testing this PR and/or a modified version of it if it does not compile it would be greatly appreciated. TIA.

@gi-man : Maybe you can help?

@phweyland phweyland force-pushed the import-copy-timestamps2 branch from 8d9a3e2 to 5ea47dc Compare August 22, 2022 18:36
@phweyland
Copy link
Copy Markdown
Contributor Author

The CI error seems to have gone ...

@gi-man
Copy link
Copy Markdown
Contributor

gi-man commented Aug 22, 2022

HEAD is now at 5ea47dc

C:/msys64/home/gman/test/darktable/src/control/jobs/control_jobs.c:2113:19: error: incompatible type for argument 2 of 'utime'
 2113 |     utime(output, times); // set origin file timestamps
      |                   ^~~~~
      |                   |
      |                   struct utimbuf

@phweyland
Copy link
Copy Markdown
Contributor Author

HEAD is now at 5ea47dc

What do you mean ? Is there an issue there ?

Pascal, what I can do is to remove the WIN32 code and let someone with access to windows dev end the job ...

@gi-man
Copy link
Copy Markdown
Contributor

gi-man commented Aug 22, 2022

I wasnt clear. It failed to build on the current UCRT environment/packages. I think utime.h is not available in the current library packages we use. @kmilos thoughts?

@phweyland phweyland force-pushed the import-copy-timestamps2 branch from 5ea47dc to a5d3920 Compare August 22, 2022 19:46
@phweyland
Copy link
Copy Markdown
Contributor Author

I've removed the WIN32 part. It was (in case it could help):

struct utimbuf times;
times.actime = statbuf.st_atime;
times.modtime = statbuf.st_mtime;
utime(output, times); // set origin file timestamps

@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Aug 22, 2022

Unfortunately not near mydev machine for a while to check the UCRT status...

@TurboGit
Copy link
Copy Markdown
Member

@kmilos : I have assigned this to you for the UCRT possible missing package. TIA.

@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Sep 2, 2022

I think utime.h is not available in the current library packages we use.

It is available from mingw-w64-headers, which should be installed already as part of the toolchain.

I wonder if some of the defined macros are creating some issue for the exported symbols though: https://github.com/mingw-w64/mingw-w64/blob/master/mingw-w64-headers/crt/sys/utime.h

@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Sep 5, 2022

@phweyland It looks like on Windows we only have seconds precision and the first utime() call:

https://man7.org/linux/man-pages/man2/utime.2.html

so you'd need to create this utimbuf struct which has two time_t fields before calling utime() instead of utimes() on other platforms.

So the Windows branch would be something like:

    struct utimbuf times;
    times.actime = statbuf.st_atime;
    times.modtime = statbuf.st_mtime;
    utime(output, &times); // set origin file timestamps

It looks like you were just missing "address of" when you tried it above?

@phweyland phweyland force-pushed the import-copy-timestamps2 branch from a5d3920 to 168f994 Compare September 5, 2022 21:08
@TurboGit
Copy link
Copy Markdown
Member

TurboGit commented Sep 6, 2022

@phweyland : Just to be sure there was no quirk on your side. Just after @kmilos suggestion you have pushed some changes but this does not have the _WIN32 part as proposed.

EDIT: Some context, this is the last issue for 4.0.1 and so when fixed we can release a new version :)

@phweyland phweyland force-pushed the import-copy-timestamps2 branch from 168f994 to bcfefd2 Compare September 6, 2022 16:51
@phweyland
Copy link
Copy Markdown
Contributor Author

you have pushed some changes but this does not have the _WIN32 part as proposed

Indeed! Sorry. I hope this is ok now.

@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Sep 6, 2022

Still missing.

@phweyland
Copy link
Copy Markdown
Contributor Author

Still missing.

Sorry again, I'd skipped your message, :(
Introducing your proposal.

@phweyland phweyland force-pushed the import-copy-timestamps2 branch from bcfefd2 to 19254b6 Compare September 6, 2022 17:23
@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Sep 6, 2022

Windows CI is green! @TurboGit

Copy link
Copy Markdown
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cooperation here!

@TurboGit TurboGit merged commit 62c20f8 into darktable-org:master Sep 6, 2022
@phweyland phweyland deleted the import-copy-timestamps2 branch September 6, 2022 19:39
@WMAM
Copy link
Copy Markdown

WMAM commented Sep 6, 2022

Thank you so much!

@TurboGit
Copy link
Copy Markdown
Member

TurboGit commented Sep 6, 2022

@phweyland : This does not merge (conflicts) on darktable-4.0.1 branch. I'm proposing to also cherry-pick 9418b74 (variable MAKER & MODEL at import), ok for you? Otherwise we need to create a specific patch for 4.0.1 branch for this PR. What do you think?

@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Sep 6, 2022

I'd test the nightly tomorrow before 4.0.1 tagging though 😉

@phweyland
Copy link
Copy Markdown
Contributor Author

I'm proposing to also cherry-pick 9418b74 (variable MAKER & MODEL at import), ok for you?

That's ok for me, sure !

@TurboGit
Copy link
Copy Markdown
Member

TurboGit commented Sep 6, 2022

Done ! So 4.0.1 should be almost ready now.

@kmilos
Copy link
Copy Markdown
Contributor

kmilos commented Sep 7, 2022

@WMAM Can you please confirm w/ the last nightly build?

@WMAM
Copy link
Copy Markdown

WMAM commented Sep 7, 2022

@kmilos Sorry for being late. We are on Pacific time, and I can barely see your message. Yes, I worked with that build last night and it worked perfectly.
Thank you so much!

@MStraeten
Copy link
Copy Markdown
Collaborator

breaks osx build:

[ 43%] Building C object bin/CMakeFiles/lib_darktable.dir/develop/blend.c.o
/Users/martinstraeten/src/darktable/src/control/jobs/control_jobs.c:2124:37: fatal error: no member named 'st_atim' in 'struct stat'
    times[0].tv_sec = (long)statbuf.st_atim.tv_sec;
                            ~~~~~~~ ^
1 error generated.
make[2]: *** [bin/CMakeFiles/lib_darktable.dir/control/jobs/control_jobs.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [bin/CMakeFiles/lib_darktable.dir/all] Error 2
make: *** [all] Error 2

git bisect bad
62c20f8 is the first bad commit
commit 62c20f8
Author: phweyland philippe.weyland@laposte.net
Date: Mon Aug 22 16:39:25 2022 -0300

import&copy: keep the original access and modified timestamps

partially (on linux) fixes #12355

src/control/jobs/control_jobs.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: DAM managing files, collections, archiving, metadata, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The "copy&import" operation results in a change in the date and time of the raw file.

6 participants