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

Support the latest newlib, libc++ and GCC (replaces #442) #470

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ppannuto
Copy link
Member

@ppannuto ppannuto commented Oct 23, 2024

#442 has an unfortunate chicken-and-egg problem wherein a commit new enough to checkout by hash in the dockerfile will only exist in the main libtock-c repo if the pull request comes from a branch within the libtock-c repo.

Hence this PR, which is based on the external branch, but is now coming from an internal branch. This isn't the most ergonomic or friendly solution, but this is also a rare enough issue [hopefully] that a little occasional human labor seems the path of least resistance.

I added commits to update the repo path, remove the unneeded patch file, and update the hash to check out.


TODO: Technically, the Makefile change to only patch if there's a patchfile is untested as newlib is still building on my laptop, but I doubt there will be any issue Hubris will get you every time... ceda51b

alevy
alevy previously approved these changes Oct 25, 2024
@alevy
Copy link
Member

alevy commented Oct 25, 2024

@ppannuto (and @bradjc & @lschuermann) does this require updating our binary repositories of pre-compiled libraries?

@lschuermann
Copy link
Member

@alevy Yes, I guess so. @bradjc I have never built these toolchains, do you have this infrastructure already set up? We can then add these new artifacts to the mirrorcheck DB which will ensure that we have this replicated everywhere.

@ppannuto
Copy link
Member Author

@lschuermann re build infra: that's what the dockerization is for; it should just be a matter of running each of the new docker-create.sh's.

That said, I'm building the toolchains currently and can send you built artifacts for the mirror when they finish.

@ppannuto
Copy link
Member Author

@bradjc @lschuermann Here are updated built artifacts for mirrors:

https://www.dropbox.com/scl/fi/26qy6b90048splzzyd42z/libtock-newlib-4.4.0.20231231.zip?rlkey=3wyfyokyoj3i5ukcb9vrotvf5&dl=0
686af44e1bba625eb24b3cfb1fd2d48a61848c1edebbd49b5dbec554ebf2ea94 libtock-newlib-4.4.0.20231231.zip

https://www.dropbox.com/scl/fi/ns3ylwnoywgecgjz3vdjy/libtock-libc-14.1.0.zip?rlkey=x79iryk0yq2eme6f381sw4d2p&dl=0
46d50ea6a380f2c977e6aad187da216c35b793b31b3ea6de9646339c2a22f13c libtock-libc++-14.1.0.zip

I think this PR should be good to go

Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

Assuming it is supposed to be libtock-libc++-14.1.0.zip I have added these to the uva mirror.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for these @ changes. Patch changes make sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is in the title of the commit: show execution of commands that can fail

When running the docker script, it was opaque which command failed if the build failed without this. (i.e., I had to make this change to debug why a build failed).

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense for debugging but now that it is debugged why would this fail? If this is failing then our docker approach doesn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, I agree, they shouldn't fail [and don't currently], but if, e.g., something changes when we add the next newlib version I'd rather not build everything twice just to turn on a few prints.

More generally, it aligns with the rest of the build system, i.e., print commands that do something [noting that for the more common / frequent builds with giant commands, we by default shorten (but do not hide) the prints for things like CC foo.c].

lschuermann added a commit to tock/mirrorcheck that referenced this pull request Nov 1, 2024
@lschuermann
Copy link
Member

Here are updated built artifacts for mirrors:

@ppannuto Sorry for the delay, added them to my mirror and the mirrorcheck URLs: tock/mirrorcheck@8cdd72e

@bradjc
Copy link
Contributor

bradjc commented Nov 1, 2024

I guess we can vote, but I feel that these are not commands to build our project but are frozen steps to reproduce a dependency. The changes are also not related to the PR title.

Copy link
Member

@lschuermann lschuermann 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 indifferent w.r.t. the @ changes. I agree they maybe shouldn't be part of this PR, but they are in a separate commit, and I don't have strong feelings about the actual changes either way.

Rest looks good.

@alevy
Copy link
Member

alevy commented Nov 2, 2024

I'm similarly agnostic regarding the @, though am sympathetic to @bradjc's kvetching over mixing this into a basically unrelated PR. I won't block either way, but I suggest @ppannuto just remove that commit (and maybe make it VERBOSE=1 option in a different PR?) in the name of peaceable coexistance and harmony

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.

5 participants