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.

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

ANSI platform builds shouldn't link pthreads. #181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ANSI platform builds shouldn't link pthreads. #181

wants to merge 1 commit into from

Conversation

waywardmonkeys
Copy link
Contributor

@waywardmonkeys waywardmonkeys commented Feb 27, 2023 •
edited by rptb1
Loading

Since this isn't using threads or POSIX locks, the -lpthreads should not be needed and having it only makes it easier to accidentally add some dependency on it.

[Edit by @rptb1 : @waywardmonkeys requires this change to build on WebAssembly. See #168 and #175 .]

@waywardmonkeys
ANSI platform builds shouldn't link pthreads.
14ce9eb 
Since this isn't using threads or POSIX locks, the `-lpthreads`
should not be needed and having it only makes it easier to
accidentally add some dependency on it.
@waywardmonkeys
Copy link
Contributor Author

It looks like the ANSI target is only tested on Travis CI (without digging deeply) and then we run into the same issue as the WebAssembly port where we're building threaded targets (some tests) in a single-threaded build.

@rptb1
Copy link
Member

rptb1 commented Feb 27, 2023

It looks like the ANSI target is only tested on Travis CI (without digging deeply)

I'm working on design.mps.ci in #145 and will try to figure out the rationale for this.

@rptb1 rptb1 requested review from rptb1 and thejayps March 6, 2023 20:43
@rptb1 rptb1 added low risk This work is or would be of low risk of introducing defects. build Problems with builds and build automation labels Mar 6, 2023
@thejayps thejayps added the essential Will cause failure to meet customer requirements. Assign resources. label Mar 20, 2023
@thejayps
Copy link
Contributor

JH+RB: essential as this is likely slowing down developers. Awaiting proc.review.express soon

@rptb1
Copy link
Member

rptb1 commented Mar 23, 2023 •
edited
Loading

Executing proc.review.express

  1. Start time 15:09.
  2. Entry criteria: entry.universal
  3. entry.universal.reason is false. Although @waywardmonkeys explained what the change was he doesn't explain why it's needed.
  4. Edited the original post to fix that.
  5. entry.universal.source-available is a bit tricky. Is it documented somewhere that WebAssembly platform does not provide pthreads?
  6. There is some stuff on the web which claims pthreads is supported for Emscripten. So the situation is unclear.
  7. Failing review entry until this situation is clarified.
  8. Procedure took 9 minutes.

@rptb1
Copy link
Member

rptb1 commented Mar 23, 2023

I'll clarify what this is all about with @waywardmonkeys , who's the expert on the WebAssembly platform family.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rptb1 rptb1 Awaiting requested review from rptb1

@thejayps thejayps Awaiting requested review from thejayps

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
build Problems with builds and build automation essential Will cause failure to meet customer requirements. Assign resources. low risk This work is or would be of low risk of introducing defects.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@waywardmonkeys @rptb1 @thejayps