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

Update depends #636

Closed

Conversation

TheComputerGenie
Copy link

new config.sub
new config.guess
set vars before trying to use them

closes #633, #634, and #635

new config.sub
new config.guess
set vars before trying to use them
@TheComputerGenie
Copy link
Author

For clarity, this is a newer patch than #634 and the fix for depends/funcs.mk fixes the issue in #633 and removes any need for #635 since the vars are set before they are needed and no need to move them to a place they don't "belong" just to get them to exist.

@DeckerSU
Copy link

DeckerSU commented Oct 5, 2024

What's the purpose of updating config.sub / config.guess right now if we're only supporting the x86_64-unknown-linux-gnu, x86_64-w64-mingw32, and x86_64-apple-darwin triplets, which are already being detected correctly? We could use the latest config.guess from git.savannah.gnu.org, but as I recall, it could introduce some issues. For instance, the canonical system name for Linux might change from x86_64-unknown-linux-gnu to x86_64-pc-linux-gnu, which would lead to unexpected behavior in some places, such as the Rust dependencies installation (which would also need to be updated).

Of course, we could update the entire build system to align with the new config.sub, but what's the real benefit of doing this right now? Is it just for the sake of "at least being in the same decade as the OSs we're guessing at," as you mentioned here?

configs were not set as executable
newer config.sub (with proper commenting link and other updates)
@TheComputerGenie
Copy link
Author

TheComputerGenie commented Oct 5, 2024

For instance, the canonical system name for Linux might change from x86_64-unknown-linux-gnu to x86_64-pc-linux-gnu

I'm not sure of a specific instance where that's relevant. The build scripts are getting their system info from the depends and the guess would be identical (x=x).
see:
build-mac.sh#L44
build-win.sh#L10
build.sh#L29-L31

Is it just for the sake of "at least being in the same decade as the OSs we're guessing at," as you mentioned here?

The point of that comment is about forward thinking. How are we to be the "bleeding edge of tech" if we build our base with a sub system that is nearly a decade old and doesn't recognize anything new since 2015? Code shouldn't be updated "for the sake of updating it", it should be updated when there are systems that it doesn't work on and those systems are to be the standard.

As for rust, the pulled file could be named blipbloptar.gz and it wouldn't effect what's done with it after (the exception to that is the hard-coded cp line for win or some other thing done similarly to avoid using the vars) as it goes into a dir and the configs will reguess the same thing which will repoint to that dir in the build (see links above).

but what's the real benefit of doing this right now?

As opposed to waiting a week before the next hard fork? Logically, "the real benefit of doing this right now" is nearly a year of testing and further development!

@DeckerSU
Copy link

DeckerSU commented Oct 5, 2024

it should be updated when there are systems that it doesn't work

On which systems exactly, that we support, does the current code not work? Can you provide some examples?

Anyway, thank you so much for your contribution and for taking the time to propose an update for config.guess and config.sub to support new OS detection. We really appreciate your effort and attention to detail in helping improve the project.

At the moment, seems the current versions of these scripts already cover all the systems we officially support. As a result, we won’t be updating them right now. However, we’ll definitely consider your changes when we revisit this in the future.

Please don’t hesitate to contribute again in the future. Your efforts are always welcome!

@TheComputerGenie
Copy link
Author

TheComputerGenie commented Oct 5, 2024

On which systems exactly, that we support, does the current code not work? Can you provide some examples?

You already did that, and it's sad that I have to point that out:

the canonical system name for Linux might change from x86_64-unknown-linux-gnu to x86_64-pc-linux-gnu

That it "technically" "works" shouldn't be the standard. That it works properly and properly identifies as many setups as possible should be the standard!

Do we, and more specifically the team, want komodo to be a thing "mostly works" if you ignore x or y being outdated or do we want komodo to be a leader in the space and a thing to aspire to be?

DeckerSU added a commit to DeckerSU/KomodoOcean that referenced this pull request Nov 6, 2024
Been a few years since we last updated these.

- bitcoin/bitcoin#27508
- KomodoPlatform/komodo#636

Taking into account that the `x86_64-unknown-linux-gnu` triplet
has been changed to `x86_64-pc-linux-gnu` with the new files.
DeckerSU added a commit that referenced this pull request Nov 6, 2024
Been a few years since we last updated these.

- bitcoin/bitcoin#27508
- #636
- DeckerSU/KomodoOcean#84

Taking into account that the `x86_64-unknown-linux-gnu` triplet
has been changed to `x86_64-pc-linux-gnu` with the new files.
@DeckerSU
Copy link

DeckerSU commented Nov 6, 2024

Thank you for PR #636! We’re closing it in favor of PR #641, which builds on your ideas. Your contribution is much appreciated! 🙏

@DeckerSU DeckerSU closed this Nov 6, 2024
DeckerSU added a commit that referenced this pull request Nov 20, 2024
Been a few years since we last updated these.

- bitcoin/bitcoin#27508
- #636
- DeckerSU/KomodoOcean#84

Taking into account that the `x86_64-unknown-linux-gnu` triplet
has been changed to `x86_64-pc-linux-gnu` with the new files.
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

Successfully merging this pull request may close these issues.

2 participants