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

date: Add option to use manual tz db #22690

Closed
wants to merge 2 commits into from
Closed

Conversation

Kaaml
Copy link

@Kaaml Kaaml commented Feb 6, 2024

Specify library name and version: lib/1.0


@CLAassistant
Copy link

CLAassistant commented Feb 6, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Kamil Mazurkiewicz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@danimtb
Copy link
Member

danimtb commented Feb 27, 2024

Hi @Kaaml, thanks for your PR.

Could you explain a bit how does this work? I mean, setting this option to True in the recipe will lead to a different package, but, is this really true? Could you show how would a consumer of this recipe indicate the path to the manual DB? Thank you!

@Kaaml
Copy link
Author

Kaaml commented Mar 2, 2024

I detected other pull requests that are modifying date/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

Hi @danimtb,

First of all, sorry for the long reply. Many users may have problem especially on windows when the date library checks for new version of IANA database and the end user doesn't have or want have 7zip in PATH variable. In my solution I put the flag to the library to block automatic download library. When this flag is set, users of the package have to remember to manually set location of IANA database.
date::set_install("/path/to/iana/database");
I saw few issues/PR for solving problem like this but all have all were more complicated and has dependency to other packages. This change can be replaced with more complex solution when will be ready.

@danimtb
Copy link
Member

danimtb commented Mar 12, 2024

Hi @Kaaml and thanks for the reply.

I am not sure I totally understand the usage of the option. Do you mean that without this new option to set a manual path to the database, users have to do date::set_install("/path/to/iana/database"); in their project?

Also, as pointed out in my previous comment, setting manual_tz_db to True/False, will lead to different packages of date, but I am not sure if the binary packages are actually different... If they aren't, then this option will need to be excluded from the pacakge id

@Kaaml
Copy link
Author

Kaaml commented Mar 13, 2024

@danimtb

Hi @Kaaml and thanks for the reply.

I am not sure I totally understand the usage of the option. Do you mean that without this new option to set a manual path to the database, users have to do date::set_install("/path/to/iana/database"); in their project?

Currently when we use date::set_install("/path/to/iana/database"); the date library just changes the path to the IANA database. If there is no IANA database under this path, or if the version is older than on the IANA website, the library will download the new version and try to extract it and replace the original version.

If we use option manual_tz_db :

  • autodownloading of IANA database will be disabled
  • the date::set_install("/path/to/iana/database"); is required; Or you need to put IANA database in deafault path whic is declared in library ( usually something like this in Windows "c:\Users\username\Downloads" )

Also, as pointed out in my previous comment, setting manual_tz_db to True/False, will lead to different packages of date, but I am not sure if the binary packages are actually different... If they aren't, then this option will need to be excluded from the pacakge id

Yes, they are diffrent. Some functions has excluded some parts of code responsible for automatic download and checking for new version of IANA.

More details
In Date CMakeLists we have defined HAS_REMOTE_API=0 and AUTO_DOWNLOAD=0 variables when we define manual_tz_db. Next in
tz.cpp file we have some #IF based on previously defined in CMakeLists.txt. The line 3569 is most important for me because it's doing auto download when file doesn't exist and also updating to new version when file exist. It's worth to notice that if we set manual_tz_db the curl library is also redundatn and would not be linked.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

}
default_options = {
"shared": False,
"fPIC": True,
"header_only": False,
"use_system_tz_db": False,
"use_tz_db_in_dot": False,
"manual_tz_db": False,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add an options_description = { "use_system_tz_db": "...", ...} attribute to the recipe. Their meaning of these options and how they interact is a bit vague otherwise.

I would also maybe call the new option allow_tz_db_download instead.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@ghost ghost mentioned this pull request May 10, 2024
3 tasks
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Failure in build 3 (de9ffc92c273054abdcaebf4dee5dba364b4b704):

  • date/3.0.1:
    Didn't run or was cancelled before finishing

  • date/3.0.0:
    Didn't run or was cancelled before finishing

  • date/2.4.1:
    Error running command conan info date/2.4.1@#896b61eca1764c3c265f0a14f0def81c --json {jsonName} --dry-build -pr {profileName}:

    [settings]
    arch=x86_64
    build_type=Release
    compiler=gcc
    compiler.libcxx=libstdc++11
    compiler.version=5
    os=Linux
    [options]
    date:header_only=False
    date:shared=False
    
    ...
    ERROR: /home/conan/workspace/prod-v1/bsr/69828/bdaec/.conan/data/date/2.4.1/_/_/export/conanfile.py: Error while initializing options. option 'allow_tz_db_download' doesn't exist
    Possible options are ['shared', 'fPIC', 'header_only', 'use_system_tz_db', 'use_tz_db_in_dot', 'allow_tz_db_download ']
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.


Conan v2 pipeline ❌

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping @conan-io/barbarians on the PR and we will help you.

Failure in build 3 (de9ffc92c273054abdcaebf4dee5dba364b4b704):

  • date/3.0.1:
    Error running command conan export --name date --version 3.0.1 recipes/date/all/conanfile.py:

    ======== Exporting recipe to the cache ========
    ERROR: Error loading conanfile at '/home/conan/workspace/prod-v2_cci_PR-22690/recipes/date/all/conanfile.py': Error while initializing options. option 'allow_tz_db_download' doesn't exist
    Possible options are ['shared', 'fPIC', 'header_only', 'use_system_tz_db', 'use_tz_db_in_dot', 'allow_tz_db_download ']
    
  • date/2.4.1:
    Error running command conan export --name date --version 2.4.1 recipes/date/all/conanfile.py:

    ======== Exporting recipe to the cache ========
    ERROR: Error loading conanfile at '/home/conan/workspace/prod-v2_cci_PR-22690/recipes/date/all/conanfile.py': Error while initializing options. option 'allow_tz_db_download' doesn't exist
    Possible options are ['shared', 'fPIC', 'header_only', 'use_system_tz_db', 'use_tz_db_in_dot', 'allow_tz_db_download ']
    
  • date/3.0.0:
    Error running command conan export --name date --version 3.0.0 recipes/date/all/conanfile.py:

    ======== Exporting recipe to the cache ========
    ERROR: Error loading conanfile at '/home/conan/workspace/prod-v2_cci_PR-22690/recipes/date/all/conanfile.py': Error while initializing options. option 'allow_tz_db_download' doesn't exist
    Possible options are ['shared', 'fPIC', 'header_only', 'use_system_tz_db', 'use_tz_db_in_dot', 'allow_tz_db_download ']
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

@uilianries
Copy link
Member

Closing this PR as the author did not sign the CLA and the option is bit too much, as ConanCenterIndex has no registered issues related to a different path for IANA database.

I would suggest using the Conan configuration tools.cmake.cmaketoolchain:extra_variables instead, so you could add extra CMake defines without changing the recipe. For instance:

-c 'tools.cmake.cmaketoolchain:extra_variables={"MANUAL_TZ_DB": "True"}'

In case still having the same problem, please, re-open the PR so we can discuss about.

@uilianries uilianries closed this Nov 27, 2024
@uilianries uilianries self-assigned this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants