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

[wasm] Fix order of include paths, to have the obj dir first #50303

Merged
merged 3 commits into from
Apr 7, 2021
Merged

[wasm] Fix order of include paths, to have the obj dir first #50303

merged 3 commits into from
Apr 7, 2021

Conversation

radical
Copy link
Member

@radical radical commented Mar 26, 2021 •
edited
Loading

this regressed recently, and we started using pinvoke-table.h, and
icall-table.h from the runtime pack instead of from the obj dir.

To fix:

  • pinvoke-table.h should not be included in the runtime pack anyway, so remove that
  • Fix the order of the include paths

@radical radical added arch-wasm WebAssembly architecture area-Build-mono labels Mar 26, 2021
@radical radical requested a review from marek-safar as a code owner March 26, 2021 22:41
@ghost
Copy link

ghost commented Mar 26, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

this regressed recently, and we started using pinvoke-table.h, and
icall-table.h from the runtime pack instead of from the obj dir.

Author: radical
Assignees: -
Labels:

arch-wasm, area-Build-mono
Milestone: -

@radical
Copy link
Member Author

radical commented Mar 26, 2021

I'll add a test for this once #49398 is merged.

@radical radical requested review from steveisok, vargaz and lewing March 26, 2021 22:42
@vargaz
Copy link
Contributor

vargaz commented Mar 26, 2021 •
edited
Loading

Maybe we should rename those files, and include them inside an ifdef to prevent this from happening.

@lewing
Copy link
Member

lewing commented Mar 27, 2021 •
edited
Loading

I'm pretty sure pinvoke-table.h is the renamed file, but for some reason it is also in the runtime pack (along side pinvoke.h)

@radical
Copy link
Member Author

radical commented Mar 29, 2021

So, should we:

  1. remove pinvoke-table.h from the runtime pack,
  2. and always use the generated pinvoke-table.h ?

@lewing
Copy link
Member

lewing commented Mar 29, 2021

I looked closer and it appears I was wrong. If we want to rename that is fine, double check what is happening with the icall tables as well.

radical added 2 commits April 6, 2021 16:53
@radical
[wasm] Fix order of include paths, to have the obj dir first
8acbb24 
this regressed recently, and we started using `pinvoke-table.h`, and
`icall-table.h` from the runtime pack instead of from the obj dir.
@radical
[wasm] Don't include pinvoke-table.h in the runtime pack.
9b55857 
`pinvoke-table.h` is generated when building the native files for wasm.
They are not required in the runtime pack, as any wasm app build needing
to do native relinking would be generating one of it's own.
@radical radical force-pushed the wasm-fix-include branch from 5ffacdd to 9b55857 Compare April 6, 2021 20:57
@radical
Copy link
Member Author

radical commented Apr 7, 2021

Maybe we should rename those files, and include them inside an ifdef to prevent this from happening.

IIUC, pinvoke-table.h is generated when building the native files for wasm.
They are not required in the runtime pack, as any wasm app build needing
to do native relinking would be generating one of its own.

So, I removed it from the runtime pack, and the rename shouldn't be needed.

@radical radical merged commit 740121d into dotnet:main Apr 7, 2021
@radical radical deleted the wasm-fix-include branch April 7, 2021 17:51
thaystg added a commit to thaystg/runtime that referenced this pull request Apr 7, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_fix_42327
b5346a6 
* upstream/main: (568 commits)
  [wasm] Set __DistroRid on Windows to browser-wasm (dotnet#50842)
  [wasm] Fix order of include paths, to have the obj dir first (dotnet#50303)
  [wasm] Fix debug build of AOT cross compiler (dotnet#50418)
  Fix outdated comment (dotnet#50834)
  [wasm][tests] Add properties to allow passing args to xharness (dotnet#50678)
  Vectorized common String.Split() paths (dotnet#38001)
  Fix binplacing symbol files. (dotnet#50819)
  Move type check to after the null ref branch in out marshalling of blittable classes. (dotnet#50735)
  Remove extraneous CMake version requirement. (dotnet#50805)
  [wasm] Remove unncessary condition for EMSDK (dotnet#50810)
  Add loop alignment stats to JitLogCsv (dotnet#50624)
  Resolve ILLink warnings in System.Diagnostics.DiagnosticSource (dotnet#50265)
  Avoid unnecessary closures/delegates in Process (dotnet#50496)
  Fix for field layout verification across version bubble boundary (dotnet#50364)
  JIT: Enable CSE for VectorX.Create (dotnet#50644)
  [main] Update dependencies from mono/linker (dotnet#50779)
  [mono] More domain cleanup (dotnet#50771)
  Race condition in Mock reference tracker runtime with GC. (dotnet#50804)
  Remove IAssemblyName (and various fusion remnants) (dotnet#50755)
  Disable failing test for GCStress. (dotnet#50828)
  ...
@antonfirsov antonfirsov mentioned this pull request Apr 12, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

@steveisok steveisok Awaiting requested review from steveisok

@vargaz vargaz Awaiting requested review from vargaz

Assignees
No one assigned
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@radical @vargaz @lewing @karelz