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.

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

chore(configure): be more defensive about SpiderMonkey location #4380

Merged
merged 3 commits into from
Jan 26, 2023

Conversation

pgj
Copy link
Contributor

@pgj pgj commented Jan 17, 2023

The configure script does not check whether SpiderMonkey actually exists at the presumed location. This may go wrong when the user has a version different from the default one. The mistake is spotted only in build time, indirectly, via missing header files. That is too late and it may not be evident for the user what the problem is.

Add a user-friendly safeguard to prevent this from happening.

@big-r81
Copy link
Contributor

big-r81 commented Jan 17, 2023

Hi, we should always port changes in configure and Makefile to the windows powershell scripts too…

@pgj
Copy link
Contributor Author

pgj commented Jan 17, 2023

Oh, and it seems I also did not consider the --skip-deps flag either.

@pgj
Copy link
Contributor Author

pgj commented Jan 17, 2023

Hi @big-r81 : I tried to mention that this is for Unix-based systems -- I do not have a Windows instance around to experiment with the changes. But I can take a chance with that.

@big-r81
Copy link
Contributor

big-r81 commented Jan 17, 2023

It's only a reminder, because it ends in like #4376 and #4377. We hopefully have our Windows CI soon in our Jenkins-Pipeline...

@pgj
Copy link
Contributor Author

pgj commented Jan 17, 2023

Sure, I am happy to consider the Windows bits as well. Maybe I can find a Windows system somewhere to poke around with this.

@pgj
Copy link
Contributor Author

pgj commented Jan 17, 2023

@big-r81 do you have any ideas why this current version fails on the CI? Initially I thought that is due to --skip-deps but by browsing the code, it seems to be unrelated.

@big-r81
Copy link
Contributor

big-r81 commented Jan 17, 2023

Are these the only possibilities for the paths? On my macOs it's

/usr/local/Cellar/spidermonkey/91.13.0_1/include/mozjs-91/

@pgj
Copy link
Contributor Author

pgj commented Jan 17, 2023

Hrm, that is interesting. I have checked the corresponding rebar scripts and only these paths were included on the C compiler flags.

@big-r81
Copy link
Contributor

big-r81 commented Jan 17, 2023

Yeah, I'm wondering too atm...

Update:
/usr/local/include/mozjs-* is symlinked by homebrew...

configure Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
@nickva
Copy link
Contributor

nickva commented Jan 18, 2023

I extracted the commit which modified the Jenkinsfile.pr into a separate PR to allow this PR to run in the CI again #4383

@pgj pgj force-pushed the install-flow-improvement branch 3 times, most recently from 055d6a9 to 6544294 Compare January 20, 2023 22:19
configure.ps1 Show resolved Hide resolved
The `configure` script does not check whether SpiderMonkey
actually exists at the presumed location.  This may go wrong when
the user has a version different from the default one.  The
mistake is spotted only in build time, indirectly, via missing
header files.  That is too late and it may not be evident for the
user what the problem is.

Add a user-friendly safeguard for Unix-like systems to prevent
this from happening.
Copy link
Contributor

@big-r81 big-r81 left a comment

Choose a reason for hiding this comment

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

+1

@janl janl merged commit 23efd8e into apache:main Jan 26, 2023
@pgj pgj deleted the install-flow-improvement branch February 11, 2023 12:35
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.

4 participants