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

fix the name in find query #3828

Closed
wants to merge 1 commit into from

Conversation

mosaicthej
Copy link

The .config installed by github's fast_float, makes the library to be exposed as FastFloat, not fast_float.

So current situation is that no matter if the library exist or not, it pulls from github remote anyways.

Also updated the version to be 6.1.3 as it's the newest release.

Description

Fixes # (issue)

The current CMakeLists would pull fast_float from github anyways irrespective if the library is installed or not.

I changed the NAME field to FastFloat so it would pick up if previously has been installed.

Type of change

  • Housekeeping

How Has This Been Tested?

Tested with a clean install of fast_float library, then build stellarium. When change the NAME, cmake won't pull from remote.

Test Configuration:

  • Operating system:
    • OS: Gentoo Linux x86_64
    • Kernel: 6.10.0-gentoo-x86_64
  • Graphics Card: Intel Raptor Lake-P [Iris Xe Graphics], 13th Gen Intel i9-13900H (20) @ 5.200GHz

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (header file)
  • I have updated the respective chapter in the Stellarium User Guide
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

The .config installed by github's fast_float, makes the library to be exposed as FastFloat, not fast_float.

So current situation is that no matter if the library exist or not, it pulls from github remote anyways.

Also updated the version to be 6.1.3 as it's the newest release.
@github-actions github-actions bot requested review from 10110111, alex-w and gzotti July 29, 2024 20:02
@alex-w alex-w added the dependencies dependencies (such like ShowMySky, QXlsx, etc.) related issues label Jul 29, 2024
@alex-w alex-w added this to the 24.3 milestone Jul 29, 2024
@10110111
Copy link
Contributor

Does it work if the user has version 6.1.0 installed?

@mosaicthej
Copy link
Author

Does it work if the user has version 6.1.0 installed?

Probably not? it probably just pull in 6.1.3 from remote then, like what we used to always have. But in this way everyone would be having 6.1.3 with stellarium.

CMakeList's VERSION specifics the minimum version, so even if i entered 6.1.0, for people having 6.1.3 it would still work (i just tested on my system, it worked).

That was not the case with the main issue, the main issue is the CamelCase not snake_case for the fast_float (I know it's confusing, the package installed in system is FastFloat but the repo is called fast_float lol).

Do we want to use 6.1.0 (and higher) or use 6.1.3 (and higher)

https://github.com/fastfloat/fast_float/releases

From the release notes, there shouldn't be any ground-breaking change, I guess this commit might be something useful? fastfloat/fast_float#167

I just think it's also a good habit to keep it with the current version, maybe I should do in 2 commits as it's 2 different purposes.

Let me know what you think

@10110111
Copy link
Contributor

Yes, I was just wondering if this is because of some name change (which some projects do, unfortunately), or simply because of my mistake. I see the following here, which makes me wonder.

add_library(fast_float INTERFACE)
add_library(FastFloat::fast_float ALIAS fast_float)

@alex-w
Copy link
Member

alex-w commented Sep 13, 2024

Any news?

@alex-w alex-w mentioned this pull request Sep 19, 2024
56 tasks
@gzotti gzotti added the need more info Read the guidelines how to report properly, or we have new questions/suggestions to try label Sep 19, 2024
@alex-w alex-w added state: waiting feedback Waiting for user feedback and removed need more info Read the guidelines how to report properly, or we have new questions/suggestions to try labels Sep 21, 2024
Copy link

Hello @mosaicthej!

We really need your feedback.

@alex-w alex-w modified the milestones: 24.3, 24.4 Sep 22, 2024
@alex-w
Copy link
Member

alex-w commented Sep 29, 2024

Hmm… I found a similar patch for FreeBSD

@DarthGandalf
Copy link
Contributor

I see the following here, which makes me wonder.

That defines how the target name will look like after successful find_package. But find_package itself looks at the filenames, which in the case of fast_float are set to FastFloat.

@10110111
Copy link
Contributor

Now that I think of it, the change of the name shouldn't have bothered me too much, since fast_float library is just a temporary workaround until all the target machines' toolchains implement std::from_chars (see 389dbd5). Thus, we can just pull the latest version and follow whatever naming it defines, without much regard to what version is installed on user's machine (it's likely that none is even available from the repos of such old operating systems).

@DarthGandalf If you can confirm that this PR fixes the issue for you, I think we can merge it.

@DarthGandalf
Copy link
Contributor

@10110111 I had opened #3949 which does essentially the same, without seeing this PR first :) This version should work too.

@alex-w alex-w added os: linux Specific issues for Linux-family OS os: bsd Specific issues for BSD-family OS labels Oct 14, 2024
@alex-w alex-w added state: outdated Outdated problems and removed state: waiting feedback Waiting for user feedback labels Oct 14, 2024
@alex-w alex-w closed this Oct 14, 2024
Copy link

This pull request is outdated, such, the problematic code replaced by new tool or feature...

@Stellarium Stellarium deleted a comment from github-actions bot Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies dependencies (such like ShowMySky, QXlsx, etc.) related issues os: bsd Specific issues for BSD-family OS os: linux Specific issues for Linux-family OS state: outdated Outdated problems
Development

Successfully merging this pull request may close these issues.

5 participants