Skip to content

tools/makemanifest.py: Allow passing flags to mpy-tool.py #5412

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 28, 2021
Merged

tools/makemanifest.py: Allow passing flags to mpy-tool.py #5412

merged 1 commit into from
Jun 28, 2021

Conversation

Jongy
Copy link
Contributor

@Jongy Jongy commented Dec 11, 2019

Use it to remain with the default MPZ_DIG_SIZE = 32 on 64-bit builds.

@dpgeorge dpgeorge added port-unix tools Relates to tools/ directory in source, or other tooling labels Dec 12, 2019
dpgeorge
dpgeorge reviewed Dec 27, 2019
View reviewed changes
ports/unix/Makefile Outdated
@@ -178,7 +178,9 @@ ifneq ($(FROZEN_MANIFEST)$(FROZEN_MPY_DIR),)
# freeze, then invoke make with FROZEN_MANIFEST=manifest.py (be sure to build from scratch).
CFLAGS += -DMICROPY_QSTR_EXTRA_POOL=mp_qstr_frozen_const_pool
CFLAGS += -DMICROPY_MODULE_FROZEN_MPY
CFLAGS += -DMPZ_DIG_SIZE=16 # force 16 bits to work on both 32 and 64 bit archs
ifneq ($(MICROPY_FORCE_32BIT),1)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this will catch all build scenarios: eg what about building on a 32-bit machine? Then this config option may not be set but the dig size should still be 16.

Copy link
Contributor Author

@Jongy Jongy Dec 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, you're right. I'll think of another way, perhaps to check the compiler version.

Without this change, since mpy-tool assumes MPZ_DIG_SIZE=16, you must define MPZ_DIG_SIZE=16 in your CFLAGS/port to have it compile. It better be automatic.

Copy link
Member

Choose a reason for hiding this comment

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

If you rebase this PR on to latest master that will include a new Travis build for 32-bit unix, which can test this.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should just remove this change.

@Jongy Jongy mentioned this pull request Jan 1, 2020
Open
8 tasks
@Jongy
Copy link
Contributor Author

Jongy commented Jan 13, 2020 •
edited
Loading

I wonder if it's required, after all - I mean, I added this because my first idea was to change mpy-tool to use digit size = 32, instead of forcing the Linux kernel port to use digit size = 16.

If we think it doesn't have any considerable performance impact, we might as well:

  1. Abandon the unix change (so unix continues with always using 16)
  2. Keep the mpy-tool patch so the Linux kernel port can force it to use 32. Or, remain with the default and add -DMPZ_DIG_SIZE=16, though I don't think it'll ever be compiled in 32-bit mode...

Hmm, you're right. I'll think of another way, perhaps to check the compiler version.

This is, apparently, not so easy as I thought. I think what most configure scripts do to check this is actually compile a test program and see if the output is 64-bit or 32-bit.

I think we can keep the mpy-tool addition, but remove the change in the unix port.

@Jongy Jongy closed this Jan 13, 2020
@Jongy Jongy reopened this Jan 13, 2020
@Jongy
Copy link
Contributor Author

Jongy commented Jan 13, 2020

Sigh, github hotkeys.

@dpgeorge
Copy link
Member

@Jongy can you please rebase on latest master so it runs the CI (there are a lot of new tests now). Then I can comment on the above.

@Jongy
Copy link
Contributor Author

Jongy commented Jun 24, 2021 •
edited
Loading

@Jongy can you please rebase on latest master so it runs the CI (there are a lot of new tests now). Then I can comment on the above.

Done @dpgeorge . I will take another look at what I did here to recall what required it. It was something in #5482.

@dpgeorge
Copy link
Member

This is erroring on qemu_mips and qeum_arm builds with:

CC build-coverage/frozen_content.c
CC coverage.c
build-coverage/frozen_content.c:38:2: error: #error "incompatible MPZ_DIG_SIZE"
   38 | #error "incompatible MPZ_DIG_SIZE"
      |  ^~~~~
make: *** [../../py/mkrules.mk:77: build-coverage/build-coverage/frozen_content.o] Error 1

dpgeorge
dpgeorge reviewed Jun 25, 2021
View reviewed changes
tools/makemanifest.py Outdated
@@ -248,6 +248,7 @@ def main():
"-f", "--mpy-cross-flags", default="", help="flags to pass to mpy-cross"
)
cmd_parser.add_argument("-v", "--var", action="append", help="variables to substitute")
cmd_parser.add_argument('--mpy-tool-flags', default='', help='flags to pass to mpy-tool')
Copy link
Member

Choose a reason for hiding this comment

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

please use double quotes " (to satisfy Black formatting)

@dpgeorge
Copy link
Member

I think we can keep the mpy-tool addition, but remove the change in the unix port.

Agreed.

@Jongy
Copy link
Contributor Author

Jongy commented Jun 27, 2021

@dpgeorge , I have:

  1. Removed the unix port change, as discussed
  2. Simplified the call to makemanifest.py - it's okay to call with empty MPY_TOOL_FLAGS because args.mpy_tool_flags.split() will evaluate to [].

and fixed other CI notes

@codecov-commenter
Copy link

Codecov Report

Merging #5412 (4ada56d) into master (7ec95c2) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5412      +/-   ##
==========================================
- Coverage   98.29%   98.28%   -0.02%     
==========================================
  Files         154      154              
  Lines       19986    19985       -1     
==========================================
- Hits        19646    19643       -3     
- Misses        340      342       +2
Impacted Files Coverage Δ
py/obj.c 96.82% <0.00%> (-0.40%) ⬇️
py/runtime.c 99.23% <0.00%> (-0.16%) ⬇️
py/objtype.c 100.00% <0.00%> (ø)

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 7ec95c2...4ada56d. Read the comment docs.

@dpgeorge dpgeorge merged commit 4ada56d into micropython:master Jun 28, 2021
@dpgeorge
Copy link
Member

Thank you!

@Jongy Jongy deleted the mpy-tool-flags branch June 28, 2021 04:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees
No one assigned
Labels
port-unix tools Relates to tools/ directory in source, or other tooling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Jongy @dpgeorge @codecov-commenter