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

Forward CI changes #2035

Merged
merged 3 commits into from
Dec 20, 2021
Merged

Forward CI changes #2035

merged 3 commits into from
Dec 20, 2021

Conversation

kmilos
Copy link
Collaborator

@kmilos kmilos commented Dec 17, 2021

Forwards parts of #2019 and #2032 to main

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #2035 (23c5100) into main (29cc981) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2035   +/-   ##
=======================================
  Coverage   61.38%   61.38%           
=======================================
  Files          96       96           
  Lines       19214    19214           
  Branches     9852     9852           
=======================================
  Hits        11794    11794           
  Misses       5096     5096           
  Partials     2324     2324           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29cc981...23c5100. Read the comment docs.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 17, 2021

Ah, this is now missing also the errno = 9999 fix from @piponazo from 0.27-maintenance, will forward that as well in the coming days.

@piponazo
Copy link
Collaborator

Hi,

The changes look good to me, however there is a failing test in the job Cygwin x64 - BuildType:Release - SHARED:ON. Do you have any idea why this might be happening? Does this error looks familiar to anybody? It is the first time I see it.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 18, 2021

A new one indeed, and it doesn't occur on 0.27, but not sure if the same test case exists there...

So we either look into it, or potentially simply drop Cygwin from CI on main?

@clanmills
Copy link
Collaborator

This is Cygwin. I think the "config file" is c:\Users\somebody\exiv2.ini and the code is looking in ~/.exiv2 (or the other way about). The code should use "exiv2 -vVg config" to determine the path to the config file.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 18, 2021

This is Cygwin. I think the "config file" is c:\Users\somebody\exiv2.ini and the code is looking in ~/.exiv2 (or the other way about). The code should use "exiv2 -vVg config" to determine the path to the config file.

Yep, I think you're on to something there. Cygwin is not "Windows" like MinGW is, it behaves more like Unix, so it must be expecting ~/.exiv2 I think...

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 18, 2021

Seems this is the culprit indeed.

Looks like I'll have to install Cygwin + its Python just to figure out how os and sys are different from MinGW Python and regular/official Windows (MSVC) CPython after all...

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

You don't need that code. There's something that executes $ exiv2 -vV and parses the output into a python object. Have a look in the version_test code. You need something like this:

import system_tests
config_file_path = system_tests.BT.verbose_version('config_path')

kevinbackhouse
kevinbackhouse previously approved these changes Dec 18, 2021
@postscript-dev
Copy link
Collaborator

@kmilos: Can you make a minor change to the manpage to explain which filename is used by Cygwin. The affected part is in section 12 CONFIGURATION FILE.

When rewriting the manpage, I discovered that this didn't work by manually using the example in section 12. I placed .exiv2 and exiv2.ini in both the current directory and the home directory but this didn't work for me (using Exiv2 0.27.4 and Exiv2 0.27.5). Perhaps someone else can use the manpage example and repeat this by hand, as the Python test has passed.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 18, 2021

Thanks Robin and Peter, I'll polish this some more.

@kmilos kmilos marked this pull request as draft December 18, 2021 19:21
@clanmills
Copy link
Collaborator

clanmills commented Dec 18, 2021

In the published build, config_path is: /home/BvSsh_VirtualUsers/.exiv2. That sounds right. The build is performed using an ssh script. The ssh server on Windows is BitVise SSH Server. So his home is /home/BvSsh_VirtualUsers.

So the cygwin "config_path" follows the unix convention and is /home/$USER/.exiv2

642 rmills@rmillsmm-local:~/gnu/github/exiv2/0.27-maintenance/build $ cd /tmp
643 rmills@rmillsmm-local:/tmp $ curl -LO --silent https://www.exiv2.org/builds/exiv2-0.27.5-CYGWIN64.tar.gz
644 rmills@rmillsmm-local:/tmp $ tar xzf exiv2-0.27.5-CYGWIN64.tar.gz 
645 rmills@rmillsmm-local:/tmp $ grep config_path exiv2-0.27.5-CYGWIN64/logs/build.txt 
config_path          /home/BvSsh_VirtualUsers/.exiv2
646 rmills@rmillsmm-local:/tmp $ 

You can obtain config_path in the test suite with the code:

import system_tests
config_path=system_tests.BT.verbose_version().get('config_path')

Using the curl/tar/find/grep magic above, the paths are:

Build config_path
exiv2-0.27.5-Darwin /Users/rmills/.exiv2
exiv2-0.27.5-2019msvc64 C:\Users\BvSsh_VirtualUsers\exiv2.ini
exiv2-0.27.5-Linux64 /home/rmills/.exiv2
exiv2-0.27.5-MinGW64 C:\Users\BvSsh_VirtualUsers\exiv2.ini
exiv2-0.27.5-CYGWIN64 /home/BvSsh_VirtualUsers/.exiv2

@postscript-dev
Copy link
Collaborator

As I don't know anything about configuring the CI, I can't approve the PR. However I looked at the manpage changes that you made and they are good. Thanks for including the update.

@clanmills
Copy link
Collaborator

@postscript-dev I've offered (in 2022) to mentor Nehal for Exiv2 release engineer. So, you shouldn't need to get involved with the CI.

I hope the Nikon Flash marathon is finished soon. I can't believe how much correspondence has been involved with that. It might have been more effort than BMFF (which was a quite a big project).

Happy Holidays. If you're in the South of England, be sure to visit and stay with us in Camberley.

@kmilos
Copy link
Collaborator Author

kmilos commented Dec 20, 2021

Happy holidays indeed!

Copy link
Collaborator

@clanmills clanmills left a comment

Choose a reason for hiding this comment

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

I'm going to approve this because I think it's OK.

That's a good trick to use base_name/join to determine config_file_path. I haven't understood the C++17 code that determines config_path. When that was changed, I would have preferred an environment string such as EXIV2_CONFIG or EXIV2_CONFIG_PATH to be used to locate the config_file.

If there's anything wrong here, the CI will behave badly (again). Very pleased to see everything simplified, documented and working well. And most importantly, understandably.

It would be good to configure codecov to relax permit small negative changes. Say -0.1% = 100 lines of code.

@clanmills clanmills merged commit d508e09 into main Dec 20, 2021
@clanmills clanmills deleted the ci_forward_main branch December 20, 2021 12:28
@kmilos kmilos added the CI Issues related with CI jobs label Dec 20, 2021
@kmilos kmilos added this to the v1.00 milestone Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Issues related with CI jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants