Skip to content

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

Merged
dpgeorge merged 1 commit into
micropython:masterfrom
Jongy:mpy-tool-flags
Jun 28, 2021
Merged

tools/makemanifest.py: Allow passing flags to mpy-tool.py#5412
dpgeorge merged 1 commit into
micropython:masterfrom
Jongy:mpy-tool-flags

Conversation

@Jongy
Copy link
Copy Markdown
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
Comment thread ports/unix/Makefile Outdated
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
Copy Markdown
Member

@dpgeorge dpgeorge Dec 27, 2019

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
Copy Markdown
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
Copy Markdown
Member

@dpgeorge dpgeorge Jan 12, 2020

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
Copy Markdown
Member

@dpgeorge dpgeorge Jun 25, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

Jongy commented Jan 13, 2020

Sigh, github hotkeys.

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Jun 23, 2021

@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
Copy Markdown
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
Copy Markdown
Member

dpgeorge commented Jun 25, 2021

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
Comment thread tools/makemanifest.py Outdated
"-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
Copy Markdown
Member

@dpgeorge dpgeorge Jun 25, 2021

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
Copy Markdown
Member

dpgeorge commented Jun 25, 2021

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

Agreed.

@Jongy
Copy link
Copy Markdown
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
Copy Markdown

codecov-commenter commented Jun 27, 2021

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
Copy Markdown
Member

dpgeorge commented Jun 28, 2021

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