Skip to content

Download boost/context asm by script - #7623

Open
twose wants to merge 1 commit into
php:PHP-8.1from
twose:sync_asm
Open

twose wants to merge 1 commit into
php:PHP-8.1from
twose:sync_asm

Conversation

@twose

@twose twose commented Nov 4, 2021

Copy link
Copy Markdown
Member

The script also automatically changes the API name of boost/context to avoid conflicts with third-party libraries.

PHP may conflict with third-party libraries which are using boost/context because all of them exports fcontext APIs. I think all boost/context users should have their own API name by adding prefixes (also including Swoole and Swow).

So I add a PHP script to download boost/context asm files and modify API names automatically, by the way, some of the code is copied from gen_stub.php.

It should be noted that the script would not remove the old dir automatically, otherwise, we need to introduce more code and have some security issues. So before we update, we must remove the context dir manually, then run the script.

@nikic

nikic commented Nov 12, 2021

Copy link
Copy Markdown
Member

Adding a script to sync these files and add prefixes sounds good to me. Some questions:

  • Why does this move everything one level down into a context/ directory?
  • This also includes files we don't use in configure. Should we filter only the used files? I'm personally okay with including everything for the sake of simplicity.
  • I didn't really get your point why the directory has to be dropped manually. Is it not possible to extract files in a way that overwrites existing files?

@twose

twose commented Nov 13, 2021 •

Copy link
Copy Markdown
Member Author
  • Why does this move everything one level down into a context/ directory?

If I understand it correctly, ASM means Assembly, not only means context/asm or fiber/asm... Assuming we introduce more ASM dependencies in the future, is it reasonable to mix all ASM files together? Or is it a bit confusing if we mix sync scripts with a lot of context asm files?

  • This also includes files we don't use in configure. Should we filter only the use files? I'm personally okay with including everything for the sake of simplicity.

I'm also okay with including everything for the sake of simplicity.
I think we now have a good balance between complexity of filter rules and num of useless files.

  • I didn't really get your point why the directory has to be dropped manually. Is it not possible to extract files in a way that overwrites existing files?

Overwriting files is also good, but it cannot handle file name changes. This situation seems to happen before. The original file may become a dead file and lie in there quietly. Do you think it's a problem worth considering?
Or if you don't mind, automatically rmdir also works fine for me, too. I'm just worried that some people might think it's dangerous.

@trowski

trowski commented Nov 23, 2021 •

Copy link
Copy Markdown
Member

This mostly looks fine to me, though I too think the extra context directory is unnecessary at the moment. Note though if files are renamed or removed, configure changes will be required, so a warning indicating that may be nice. This way a future release manager doing an update is aware of the problem, as the change may not apply to their system so the build would still succeed for them.

EDIT: I guess the context dir is nice for doing an automatic rmdir, which is fine IMO. The user still has to commit the changes, so I don't see that as a problem.

@twose

twose commented Nov 23, 2021

Copy link
Copy Markdown
Member Author

@trowski Thanks for your suggestion. Does it look better now? It will move the old dir to the temp automatically.

@cmb69

cmb69 commented Dec 19, 2021

Copy link
Copy Markdown
Member

This way a future release manager doing an update is aware of the problem, […]

Is this really meant for release managers, or for updating boost/context from time to time (by whomever)?

@trowski

trowski commented Jan 3, 2022

Copy link
Copy Markdown
Member

@cmb69 It's meant for anyone wishing to update Boost.Context. My assumption was that would be release managers, but I'm probably mistaken. It can be anyone that feels compelled to do so. I intended to do this manually as these asm files are infrequently updated, but since @twose put in the work to write an automated script we could make it part of another process so updates are bundled sooner.

The script also automatically changes the API name of boost/context to avoid conflicts with third-party libraries
@twose

twose commented Jan 4, 2022

Copy link
Copy Markdown
Member Author

After reading related discussions, I tried to take some time to update this PR.

Some changes:

  • No longer move everything one level down.
  • It can unlink old files automatically now.
  • update file filter, it can load possible asm file names from the config file, which means that it no longer includes unnecessary files (just some simple tricks, but works well for me).

@joshtrichards

Copy link
Copy Markdown

Related: #20438

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