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

Removing scons and adding an error message #73

Merged
merged 11 commits into from
Apr 4, 2023
Merged

Conversation

MauriceHendrix
Copy link
Contributor

This removes Scons code, except for the error message thrown when you try to build with Scons.
However I stilll need to know where to point to, which relates to #59

@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #73 (7de7d0a) into develop (b12063c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop      #73   +/-   ##
========================================
  Coverage    99.99%   99.99%           
========================================
  Files          931      931           
  Lines        45865    45865           
========================================
  Hits         45862    45862           
  Misses           3        3           
Impacted Files Coverage Δ
global/src/OutputFileHandler.cpp 99.00% <ø> (ø)
heart/src/odes/CellMLToSharedLibraryConverter.cpp 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MauriceHendrix MauriceHendrix marked this pull request as ready for review March 2, 2023 10:05
@MauriceHendrix MauriceHendrix requested a review from jmpf March 2, 2023 10:18
Copy link
Contributor

@jmpf jmpf left a comment

Choose a reason for hiding this comment

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

Deleted files are now redundant so PR approved.

  • SConstruct error message is a placeholder and should (later) be a real URL to the Wiki
  • Subfolders python/hostconfig and python/hostconfig/machines can now be removed too

@jmpf
Copy link
Contributor

jmpf commented Mar 2, 2023

Also todo: remove mentions of Scons in code comment instructions.

@MauriceHendrix MauriceHendrix mentioned this pull request Mar 2, 2023
@fcooper8472
Copy link
Member

This looks good to me.

@MauriceHendrix MauriceHendrix merged commit 6dddeba into develop Apr 4, 2023
@MauriceHendrix MauriceHendrix deleted the remove_scons branch April 4, 2023 08:55
@@ -54,13 +54,6 @@ OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
*
*
* For example:
Copy link
Member

Choose a reason for hiding this comment

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

This really wants replacing with cmake example rather than just deleting

@@ -15,19 +15,6 @@ our special binary version of the Triangle/Tetgen format.
to containing element indices. The presence of this file cuts the calculation cost at mesh load time.
* If a Chaste format fibre file (.axi or .ortho) is associated with the mesh then this will also be converted to our binary format.

=== Making the executable(s) ===
Copy link
Member

Choose a reason for hiding this comment

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

This really wants replacing with cmake example rather than just deleting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought naking standalone excutables wasn't possible anymore? @fcooper8472 ? unless I have missunderstood?

Copy link
Member

Choose a reason for hiding this comment

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

We don't try and make them for people due to difficulties getting compatible cross-platform ones for Linux (used to generate the cardiac executable for download) but still should be able to make them (like ApPredict).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's still absolutely possible. We no longer ship a cardiac executable that you can expect to run portably without compiling it yourself, but the top level apps directory (and indeed the apps directory in any user project) can have standalone executables with their own int main() methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok but how do we write that in the tutorial?

Copy link
Member

Choose a reason for hiding this comment

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

This would just be the corresponding instructions but using CMake, I suppose?

scons chaste_libs=1 compile_only=1 exe=1 build=GccOpt apps

would become, assuming you have configured Chaste

make Chaste

or

make MeshConvert

and the build=GccOpt_ndebug equivalent would be passing -DCMAKE_BUILD_TYPE=Release at configure time.

@MauriceHendrix
Copy link
Contributor Author

addressing the new comments about build properties for cmake in #94

jmpf added a commit that referenced this pull request Apr 28, 2023
Removing scons and adding an error message
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.

None yet

5 participants